-
Notifications
You must be signed in to change notification settings - Fork 137
Add regex for path matching #3874
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
Conversation
Hi @fabian4! Thanks for opening this pull request! |
2202934
to
72227ea
Compare
Hey @fabian4, thanks a lot for your contribution to NGINX Gateway Fabric! I noticed there are still some parts left in this PR—were you looking for initial feedback before continuing development, or are you planning to submit this in parts? |
Hi @salonichf5, thanks a lot for the comment! I kept this PR as a draft since there are still some parts I’d like to finish up. In the meantime, getting some early feedback would be super helpful — that way I can adjust things early and make sure I’m heading in the right direction. Really appreciate your input! |
72227ea
to
54135b5
Compare
c76e809
to
63c06fe
Compare
For sure, we will get some eyes on it. Thank you again for the contribution. |
@fabian4 We'll be sure to review this soon! In the meantime, I ran the pipeline and there are some linting/unit test errors. Also, please be sure to follow this guide for formatting your main commit message and PR description. And please update the release note section the PR description as well, so we can include a notein our CHANGELOG when this ultimately gets released. |
194ac10
to
ff24ee1
Compare
ff24ee1
to
7f70c31
Compare
https://github.com/nginx/nginx-gateway-fabric/actions/runs/17919926662/job/50987553809?pr=3874 BTW I can't figure why above test are failing since I can't get any thing like that when I run unit test and lint on my env. It says the |
@fabian4 Yeah I saw similar issues on my fork recently as well, not sure what's happening there. We'll investigate. |
@fabian I just had the first successful run for my fork test PR, so hopefully the pipeline issues should resolve for you now. I'll get on to testing your PR once we can run the linter and tests in the pipeline. |
7f70c31
to
895c15c
Compare
Testing done on the branch Error expected cases:
Case 1: Drops its if starts with
Case 2: unescaped $
Case 3: Very long pattern
|
Testing done on the branch Checking functionality that Regex works as expected
|
We need to also validate preexisting behaviour for URLRewrite, Redirect and other filters. |
@fabian4 Thank you for your patience. We are currently testing Regex thoroughly—both on its own and in combination with our other filters. I really appreciate your contribution. From my initial round of testing, everything looks good overall, with only a few exceptions that we’d like to discuss as a team to decide whether we want to allow them or not. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving to get this over the line before code freeze - we can follow up with a PR to fix the panic. Thanks so much @fabian4 for all your hard work on this!!
I can fix it later if it is not in a hurry |
Problem: As regex path matching is missing support for NGF. Solution: Add support for regex path matching, only allow a full path rewrite/redirect after the regex path match. Testing: Add additional unit test case for regex path match.
b200768
to
7771846
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah one of us will do the fix for panic
I see you fixed the panic already, amazing, thanks so much @fabian4 ! I'm rerunning the pipeline and we'll get this merged in 🎉 |
Proposed changes
Problem: Add regex for path matching
external (regex path match without any head/method/queryparams condition)
Closes #3110
Checklist
Before creating a PR, run through this checklist and mark each as complete.
Release notes
If this PR introduces a change that affects users and needs to be mentioned in the release notes,
please add a brief note that summarizes the change.