-
Notifications
You must be signed in to change notification settings - Fork 600
fix: disable -shadow host suffix append
#7229
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
Conversation
|
I am kinda confused on how to write test cases for this one. Specially the part on how to confirm that the host header in mirrored HTTP message does not contain the shadow suffix. I am reading existing test cases, however I would appreciate any help on this. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7229 +/- ##
==========================================
+ Coverage 71.07% 71.14% +0.07%
==========================================
Files 228 228
Lines 40825 40827 +2
==========================================
+ Hits 29015 29048 +33
+ Misses 10104 10079 -25
+ Partials 1706 1700 -6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@shreealt we discussed this in today's meeting. The decision is to make a breaking change to remove the Feel free to proceed. |
| RuntimeFraction: mp, | ||
| Cluster: policy.Destination.Name, | ||
| RuntimeFraction: mp, | ||
| DisableShadowHostSuffixAppend: true, |
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.
Nit: can we add a line of comment to explain why setting this option?
RequestMirror is in a Gateway API feature, so we rely on the gateway api conformance test to verify it in the e2e tests. For this PR, it's fine to verify it in the xds translator tests. |
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.
Could you add a note to the "breaking changes" section of the release-notes/current.yaml file?
|
/retest |
efb6afa to
77e3df6
Compare
zhaohuabing
left a comment
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.
LGTM. Thanks!
* fix: disable `-shadow` host suffix append Signed-off-by: Shreemaan Abhishek <shreemaanabhishek@apache.org> Signed-off-by: Adam Buran <aburan28@gmail.com>
What type of PR is this?
bugfix
What this PR does / why we need it:
The current HTTPRoute Request Mirroring automatically appends a “-shadow” to the mirror requests Host header. According to the Envoy doc, this is used for logging.
However, this can be surprising to users, as the Host header is modified without explicit notice. It may also cause issues for application that depends on the original Host header.
Which issue(s) this PR fixes:
Fixes #7227
Release Notes: Yes/No