fix: annotation konghq.com/rewrite for multiple Ingresses routing to the same Service

[programmer04]

What this PR does / why we need it:

Fixes misbehavior when multiple Ingresses point to the Same service, some of them have konghq.com/rewrite and some do not (or a different path specified in it):

• commit 1cfe028 improves integration test TestIngressRewriteURI to catch situation reported in Rewrite annotation causing KIC sync loop #5021
• commit 857cdbb contains fix - request-transformer is attached to the Kong route instead of Kong service

Which issue this PR fixes:

Fixes #5021

Special notes for your reviewer:

One case that is not covered by a test is Ingress which uses only the default backend and annotation konghq.com/rewrite is specified. It will be covered in a separate PR after merging it.

PR Readiness Checklist:

Complete these before marking the PR as ready to review:

• the CHANGELOG.md release notes have been updated to reflect any significant (and particularly user-facing) changes introduced by this PR

[codecov]

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (21a05ab) 77.8% compared to head (4efce19) 77.8%.
Report is 2 commits behind head on main.

Files	Patch %	Lines
...rnal/dataplane/translator/subtranslator/ingress.go	86.9%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##            main   #5171     +/-   ##
=======================================
- Coverage   77.8%   77.8%   -0.1%     
=======================================
  Files        168     168             
  Lines      18896   18898      +2     
=======================================
  Hits       14712   14712             
+ Misses      3354    3352      -2     
- Partials     830     834      +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[rainest]

Approving but disabled auto-merge. The test strategy looked kinda overcomplicated even if functional. I'm not sure there's a good justification I'm not seeing; you may want to simplify it if not.

It seems like the test could be simpler by just checking the plain Ingress after confirming the rewritten one works as expected, rather than having the goroutine constantly checking it in the background. Routing should not be stochastic. If the rewrite Ingress routes are active, they will always match any requests they can match, and the no-rewrite Ingress' routes will continue to match any requests they can match, with maybe the exception of more precise matches now going to the rewrite Ingress routes.

Were you actually seeing some random distribution between the two? That would probably indicate that the routes are unintentionally designed with an overlap and the test is checking something beyond just whether rewrites are not applied to the entire Service.

The use of Prefix for the standard route is maybe causing some weirdness due to it generating a regex route and running into the priority problems discussed in #3394, though even then there should be a deterministic tiebreaker (creation time breaks tie, which is bad, but is at least deterministic).

We can probably avoid integration tests for regressions like this altogether--we don't expect the plugin to apply to the entire service because of some inherent behavior of the plugin, we just configured it in the wrong place originally. The unit test should be sufficient to confirm that we don't accidentally swap it back in the future.

[programmer04]

It seems like the test could be simpler by just checking the plain Ingress after confirming the rewritten one works as expected, rather than having the goroutine constantly checking it in the background.

Due to the flapping nature of the originally reported issue #5021 just checking it one time would sometimes pass such a test. See the description of #5140. The traffic used to be distributed randomly between backends. So basically it would be a flake.

So basically the below due to bug

Routing should not be stochastic. If the rewrite Ingress routes are active, they will always match any requests they can match, and the no-rewrite Ingress' routes will continue to match any requests they can match, with maybe the exception of more precise matches now going to the rewrite Ingress routes.

wasn't true.

we configured it at wrong place orginally

It's true and we had simpler integration test and unit tests and didn't catch it. Since it was overlooked during implementation and reported as a bug by an actual user my rule of thumb is to be rather overprotective.

In a unit test, you don't see at the first glance the consequence of moving this plugin from route to the service and vice-versa (otherwise we wouldn't have such bug in the first place).

[pmalek]

@programmer04 Do you mind taking a look at this failure: https://github.com/Kong/kubernetes-ingress-controller/actions/runs/6890425270/job/18743620998#step:6:8958

It seems that it's caused by this change:

    ingress_test.go:1181: 
        	Error Trace:	/home/runner/work/kubernetes-ingress-controller/kubernetes-ingress-controller/test/integration/ingress_test.go:1181
        	Error:      	Received unexpected error:
        	            	expected >= 99% of requests to be successful, current rate is 94.736842 %, unexpected status codes: map[status code: 503:1]
        	Test:       	TestIngressRewriteURI
        	Messages:   	for Ingress without rewrite run in background

[team-k8s-bot]

The backport to release/2.12.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-release/2.12.x release/2.12.x
# Navigate to the new working tree
cd .worktrees/backport-release/2.12.x
# Create a new branch
git switch --create backport-5171-to-release/2.12.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 12a81857475d5df4662ef2959e61ccbdefa1b1dc
# Push it to GitHub
git push --set-upstream origin backport-5171-to-release/2.12.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-release/2.12.x

Then, create a pull request where the base branch is release/2.12.x and the compare/head branch is backport-5171-to-release/2.12.x.

[team-k8s-bot]

The backport to release/3.0.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-release/3.0.x release/3.0.x
# Navigate to the new working tree
cd .worktrees/backport-release/3.0.x
# Create a new branch
git switch --create backport-5171-to-release/3.0.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 12a81857475d5df4662ef2959e61ccbdefa1b1dc
# Push it to GitHub
git push --set-upstream origin backport-5171-to-release/3.0.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-release/3.0.x

Then, create a pull request where the base branch is release/3.0.x and the compare/head branch is backport-5171-to-release/3.0.x.