-
Notifications
You must be signed in to change notification settings - Fork 8.4k
Location regex should respect Exact paths #13374
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
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for kubernetes-ingress-nginx canceled.
|
This issue is currently awaiting triage. If Ingress contributors determines this is a relevant issue, they will accept it by applying the The 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-sigs/prow repository. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: dacohen 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 |
Welcome @dacohen! |
Hi @dacohen. Thanks for your PR. I'm waiting for a kubernetes 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-sigs/prow repository. |
If a location is marked as Exact, and the ingress uses regex matching, the resulting location block should respect this.
e40fb53
to
504dfb0
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.
Pull Request Overview
This PR updates the handling of exact paths when regex matching is enabled, ensuring that exact location blocks are generated correctly in the nginx configuration. Key changes include new e2e test cases validating exact path behavior, updates to the rewrite annotation tests to use terminating regex patterns, and modifications to both the location building logic and its tests.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
test/e2e/ingress/pathtype_exact.go | Added tests checking that exact paths are respected under regex matching conditions |
test/e2e/annotations/rewrite.go | Updated location regex patterns to include end-of-string markers where needed |
internal/ingress/controller/template/template_test.go | Extended tests to cover the new behavior for exact paths |
internal/ingress/controller/template/template.go | Modified the buildLocation function to generate regex with '$' for exact paths when regex matching is enforced |
|
||
if location.PathType != nil && *location.PathType == networkingv1.PathTypeExact { | ||
if enforceRegex { | ||
return fmt.Sprintf(`~* "^%s$"`, path) |
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.
[nitpick] Consider adding an inline comment above this branch to explain that appending '$' ensures the regex matches exactly and avoids unintended partial matches when enforceRegex is true.
Copilot uses AI. Check for mistakes.
If a location is marked as Exact, and the ingress uses regex matching, the resulting location block should respect this.
What this PR does / why we need it:
Even when an ingress has regex matching enabled, Exact paths should still be respected. Currently, they're ignored, which leads to non-intuitive behavior.
Types of changes
Which issue/s this PR fixes
fixes #11397
How Has This Been Tested?
A test case was added to validate that the change generates the expected location block in the resulting nginx config.
Checklist: