Skip to content
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

fix: annotation konghq.com/rewrite always works #6626

Merged
merged 2 commits into from
Nov 8, 2024
Merged

Conversation

programmer04
Copy link
Member

@programmer04 programmer04 commented Nov 6, 2024

What this PR does / why we need it:

Fixes bug reported by user description of the problem and steps to reproduce it robustly are here #6542 (comment)

Which issue this PR fixes:

Fixes #6542

Special notes for your reviewer:

This feature was covered with a quite complicated integration test TestIngressRewriteURI designed to catch exactly such a bug, but it failed to do so. Why?

  • 8e5f883
    Changes in the test have no effect (it's just an opportunistic cleanup not to have redundant stuff). The bug was hard to catch due to the order of items in the Go map during iteration - it's random and it even may be randomized for each iteration. Hence this commit always enforces the same order for defined routes by sorting them (IMHO KIC should always operate in a consistent and reproducible manner so paying the cost of sorting is worth it). This change leads to the consistent failure of the test TestIngressRewriteURI as it is supposed to be.

  • 6898b9c
    Actual one-line fix return -> continue to not ignore a rewrite annotation that is not attached to the first item in a list. It resolves the problem (as I said even without changes from the first commit).

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

Copy link

codecov bot commented Nov 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.6%. Comparing base (030db71) to head (22c892d).
Report is 4 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##            main   #6626     +/-   ##
=======================================
+ Coverage   72.2%   77.6%   +5.4%     
=======================================
  Files        202     201      -1     
  Lines      25817   23841   -1976     
=======================================
- Hits       18643   18508    -135     
+ Misses      6159    4378   -1781     
+ Partials    1015     955     -60     

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

@pull-request-size pull-request-size bot added size/S and removed size/XS labels Nov 7, 2024
@programmer04 programmer04 marked this pull request as ready for review November 7, 2024 18:31
@programmer04 programmer04 requested a review from a team as a code owner November 7, 2024 18:31
@programmer04 programmer04 merged commit ef2f1e6 into main Nov 8, 2024
42 checks passed
@programmer04 programmer04 deleted the fix-rewrite-bug branch November 8, 2024 01:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rewrite annotation not working on every request when multiple Ingresses share same backing service
2 participants