fixed rewrites for paths not ending in / #2899
Conversation
k8s-ci-robot
added
the
size/S
|
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
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. I understand the commands that are listed here. |
k8s-ci-robot
added
the
cncf-cla: no
|
CLA check fails because committed with the wrong email address. Will fix in a couple of hours. |
|
CLA signed |
k8s-ci-robot
added
cncf-cla: yes
| @@ -136,7 +136,7 @@ proxy_pass http://upstream-name; | |||
| "/", | |||
| `~* ^/something\/?(?<baseuri>.*)`, | |||
| ` | |||
| rewrite (?i)/something/(.*) /$1 break; | |||
| rewrite (?i)/something(.*) /$1 break; | |||
There was a problem hiding this comment.
These look a bit dangerous to me, the regexp should match either an end of line ($) or /.*, but not /somethingelse as it would be the case here.
There was a problem hiding this comment.
You are correct. I've changed it to only match on end of line or /.*
I'm still having trouble getting the tests to run without issues. The unit tests indicate FAIL, but without any specifics and the e2e tests I've not been able to run successfully at all yet. I did however test an image build from my fork/branch, tested it in our own environment and there it seems to do the right thing.
|
/hold |
k8s-ci-robot
added
the
do-not-merge/hold
|
/hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
Codecov Report
@@ Coverage Diff @@
## master #2899 +/- ##
==========================================
+ Coverage 47.57% 47.58% +<.01%
==========================================
Files 77 77
Lines 5532 5533 +1
==========================================
+ Hits 2632 2633 +1
Misses 2560 2560
Partials 340 340
Continue to review full report at Codecov.
|
|
@antoineco I've made the changes you suggested and tests now complete successfully. Can you have another look? Thanks! |
|
@jeroenvand please squash the commits |
|
Commits squashed |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: antoineco, jeroenvand The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
| %v%v %s%s; | ||
| %v`, path, location.Path, xForwardedPrefix, proxyPass, proto, upstreamName, abu) | ||
| } | ||
|
|
||
| return fmt.Sprintf(` | ||
| rewrite (?i)%s(.*) %s/$1 break; | ||
| rewrite (?i)%s$ %s/ break; |
There was a problem hiding this comment.
what is the reason for the target to end with slash? forgive me if it's already explained elsewhere but i can't seem to find it.
related issue: #1399
There was a problem hiding this comment.
Also confused.
Any target path that ends without slash cannot be matched in this way.
What this PR does / why we need it:
Make sure that rewrites work correctly for paths without terminating slash.
Which issue this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close that issue when PR gets merged): fixes #fixes #2845.
Special notes for your reviewer:
I did update the unit tests and it runs with errors relating to the rewrite stuff, but I couldn't get the e2e tests to run without failing. Then again, those failed with a clean master branch checkout as well so maybe I did something wrong there altogether.