-
Notifications
You must be signed in to change notification settings - Fork 438
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
Add gep 2694 support match by RegularExpression #2737
base: main
Are you sure you want to change the base?
Conversation
Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Hi @rokkiter. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@youngnick #2177 (comment) regex capture groups does not seem to be a necessary feature. The same result for regex capture groups can be achieved with the regex syntax (which can be a bit tricky) |
Thanks for getting this started @rokkiter, this is a great start. For other reviewer's clarity, here's what I had in the original comment that kicked off this GEP and PR: The biggest reason why this is not done yet is about regular expression compatibility. An GEP that addresses this will need also to address: what flavors of regexes are supported across most if not all Gateway API implementations
Other concerns I'd like to see addressed in this GEP that aren't regex compatibility:
For the last two items, we don't need to answer those in the GEP, but if we aren't going to, we need to explicitly put them in as "reserved for future work", so the discussion doesn't get derailed. The regex flavor discussions has all the hallmarks of one that could be contentious to me, which is concerning. I think that I wasn't clear enough in that statement about a few things:
Probably the biggest question I have though is: I don't think it's clear enough where the proposed struct will be used. To be clear, I think that regex support needs to be added to the path type of the Then, the rewrite and redirect filters need to have a specification for how they can use information that has been captured in a regex match (or, if they can only use the entire matched path). To put this another way, I think that the match part of a regex should go in the path match section, and the replace part should be specified in the redirect or rewrite config. So we're splitting a standard PCRE-style request that looks like I also appreciate your comments above about priority and capture groups, those are comments that should also be included in the "API" section - which should describe why this feature should work this way. On the specifics there:
Sorry, that was more than I intended to write, but hopefully this gives you a bit of a TODO list. Interesteed to hear what other GEP reviewers have to say though. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: rokkiter The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Thanks so much for making so many very meaningful suggestions. I didn't make it obvious in the title and description that this gep targets |
What type of PR is this?
/kind gep
What this PR does / why we need it:
HTTPRoute support match by RegularExpression
Which issue(s) this PR fixes:
Fixes #
#2694
Does this PR introduce a user-facing change?: