http: applying route header rules on local replies. #13204
Conversation
Signed-off-by: Alyssa Wilk <[email protected]>
| @@ -812,7 +812,16 @@ void FilterManager::sendLocalReplyViaFilterChain( | |||
| Utility::sendLocalReply( | |||
| state_.destroyed_, | |||
| Utility::EncodeFunctions{ | |||
| modify_headers, | |||
| [this, modify_headers](ResponseHeaderMap& headers) -> void { | |||
| if (streamInfo().route_entry_ && | |||
There was a problem hiding this comment.
Is it possible to make a test for the case where route_entry_ is nullptr?
There was a problem hiding this comment.
every early response test (invalid HTTP/1.1 sending 400 before we have headers and can select route) tests this. Did you want a unit test as well, or just to make sure we had coverage?
There was a problem hiding this comment.
Just wanted to make sure we have coverage
| if (streamInfo().route_entry_ && | ||
| Runtime::runtimeFeatureEnabled( | ||
| "envoy.reloadable_features.always_apply_route_header_rules")) { | ||
| streamInfo().route_entry_->finalizeResponseHeaders(headers, streamInfo()); | ||
| } | ||
| if (modify_headers) { | ||
| modify_headers(headers); | ||
| } |
There was a problem hiding this comment.
This stanza is duplicated verbatim from above and is a little tricky so might be worth de-deduping but up to you.
|
Hello. I'm not familiar with your backport policy and I'm curious if are you going to backport this bug to 1.5? |
|
We can cc @envoyproxy/stable-maintainers and ask them what they think! |
|
Since this is neither security nor stability fix, it doesn't qualify for backporting. |
|
Right, the question more if we consider it an enhancement, or a bugfix fixing inconsistent header handling. |
Commit Message: http: applying route header rules on local replies.
Risk Level: Medium (L7 change)
Testing: updated integration tests
Docs Changes: n/a
Release Notes: inline
Fixes #4317
Runtime Guard: envoy.reloadable_features.always_apply_route_header_rules