fix the bug #2799, add prefix (?i) in rewrite statement.

[teresadq]

the location in nginx.conf is case insensitive, but when we use the annotation nginx.ingress.kubernetes.io/rewrite-target in ingress rule. the rewrite statement is case sensitive. so we can fix this bug in adding (?i) in rewrite statement.

fixes #2799

[teresadq]

@antoineco Hi, please check it.

[aledbf]

@dongqi1990 please add e2e tests for this change. Thanks

[teresadq]

@aledbf ok, You means that I should modify the rewrite statement in template_test.go, like "rewrite /something/(.*) /$1 break;" in "tmplFuncTestcases" (e.g. changing "rewrite /something/(.*) /$1 break;" to "rewrite (?i)/something/(.*) /$1 break;")? Otherwise the go test doesn't pass.

[aledbf]

No, I mean, you need to add new e2e tests to check this works with a running ingress controller. Doing unit tests for this is not enough.

[aledbf]

Also, you need to change the mentioned unit tests

[codecov-io]

Codecov Report

Merging #2808 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master    #2808   +/-   ##
=======================================
  Coverage   47.47%   47.47%           
=======================================
  Files          75       75           
  Lines        5413     5413           
=======================================
  Hits         2570     2570           
  Misses       2519     2519           
  Partials      324      324

Impacted Files	Coverage Δ
internal/ingress/controller/template/template.go	66.52% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 48ee93f...8da4594. Read the comment docs.

[teresadq]

@aledbf @antoineco I had modify the unit test template_test.go and e2e test file rewrite_log.go. Please check it.

[antoineco]

Thanks for that. We also need to test this with an actual HTTP request (see other e2e examples), eg. with capital letters in the request, and assert the response looks as expected.

[teresadq]

@antoineco OK, I had two question to ask you.

1. Could you give me some reference material about e2e test. I am not familiar with it. Thank you.
2. Did I need to new file in e2e folder? If I add new feature like nginx cache or other features in nginx level 4.

[antoineco]

@dongqi1990 sorry I totally missed your response! 🙇

I don't have formal instructions for writing e2e tests but if you look in the directory you mentioned you will find out that all tests follow the same logic:

• create an Ingress + echoheaders backend
• set annotation(s) on Ingress
• wait for NGINX config refresh
• send HTTP request to NGINX
• assert the result using a Gomega matcher

I can adapt the test for you if you want to see how I would do it, or do you prefer to give it a try yourself?

[teresadq]

@antoineco Thank you very much for your kindness. I had ran the e2e test in my local machine and sometimes it works and sometimes it doesn't. Oh, I also worked very slowly.
It would be great If you can adapt the test.

[antoineco]

Will do it today and notify you.

[teresadq]

OK, ^_^

[antoineco]

Sorry I don't have enough permissions to push to your branch, so I'll send you a patch per email instead :)

[teresadq]

@antoineco My email is [email protected]. Thanks

[antoineco]

Actually no need for email, GitHub can perfectly generate patches: [PATCH] Add e2e test for rewrite-target annotation.

All you have to do is execute
git apply <(curl -sSL https://github.com/antoineco/ingress-nginx/commit/85f6d5a47ff9e15350e92685e122e8215f4ca23d.patch)
then commit the changes.

[teresadq]

OK, I will study the code these days.

[antoineco]

@dongqi1990 I'm re-running e2e tests.

[teresadq]

@antoineco I'm curious that the same code sometimes goes through the e2e test and sometimes doesn't. Why?

[antoineco]

Mostly the randomness and instability of public CI environments, most of the time it's just a timing issue. If the same test fails often we try to mitigate timing issues with extra wait conditions, etc. But usually it's pretty stable.

[antoineco]

/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: antoineco, dongqi1990

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:

• OWNERS [antoineco]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment