-
Notifications
You must be signed in to change notification settings - Fork 20
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: getting headers for OnLog #770
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #770 +/- ##
==========================================
+ Coverage 87.70% 87.89% +0.19%
==========================================
Files 136 136
Lines 8163 8163
==========================================
+ Hits 7159 7175 +16
+ Misses 783 766 -17
- Partials 221 222 +1 ☔ View full report in Codecov by Sentry. |
@@ -334,7 +334,7 @@ func (m *filterManager) localReply(v *api.LocalResponse, decoding bool) { | |||
} | |||
|
|||
func (m *filterManager) DecodeHeaders(headers capi.RequestHeaderMap, endStream bool) capi.StatusType { | |||
if !supportGettingHeadersOnLog && m.DebugModeEnabled() { | |||
if supportGettingHeadersOnLog && m.DebugModeEnabled() { |
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.
Should be
if supportGettingHeadersOnLog && m.DebugModeEnabled() { | |
if !supportGettingHeadersOnLog { |
@@ -696,7 +696,7 @@ func (m *filterManager) DecodeTrailers(trailers capi.RequestTrailerMap) capi.Sta | |||
} | |||
|
|||
func (m *filterManager) EncodeHeaders(headers capi.ResponseHeaderMap, endStream bool) capi.StatusType { | |||
if !supportGettingHeadersOnLog && m.DebugModeEnabled() { | |||
if supportGettingHeadersOnLog { |
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.
Ditto
@@ -23,7 +23,7 @@ import ( | |||
) | |||
|
|||
const ( | |||
supportGettingHeadersOnLog = false | |||
supportGettingHeadersOnLog = 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.
Only the unreleased Envoy version supports getting headers OnLog:
supportGettingHeadersOnLog = 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.
I would like to ask why supportGettingHeadersOnLog = true in envoydev and false in !envoydev, do I need to leave it as is?
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.
Well I guess I should have known, since !envoydev itself provides the ability to cache the headers on the go side without us needing to judge it by !supportGettingHeadersOnLog, I'll leave them as they are and just remove the && m.DebugModeEnabled()
.
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.
envoydev => unreleased Envoy version, support getting headers in OnLog phase.
Related to #740
The sentinel plugin needs to get the response headers in the OnLog