Local reply and upstream response rewrites using the FilterManager #14926
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -862,6 +862,12 @@ void FilterManager::sendLocalReplyViaFilterChain( | |
| // a no-op. | ||
| createFilterChain(); | ||
|
|
||
| // A bit awkward, but save the local reply data for later. This will come in handy. | ||
| // When this pointer is non-null, it means a local reply has been initiated, which | ||
| // is important later on. | ||
| local_reply_data_ = std::make_unique<Utility::LocalReplyData>( | ||
| Utility::LocalReplyData{is_grpc_request, code, body, grpc_status, is_head_request}); | ||
|
|
||
| Utility::sendLocalReply( | ||
| state_.destroyed_, | ||
| Utility::EncodeFunctions{ | ||
|
|
@@ -875,13 +881,14 @@ void FilterManager::sendLocalReplyViaFilterChain( | |
| modify_headers(headers); | ||
| } | ||
| }, | ||
| // Local reply rewrites (and upstream rewrites, for that matter) happen on the filter | ||
| // chain now, so we do not need to do any additional rewriting here. | ||
| /*rewrite_=*/nullptr, | ||
| // Local gRPC replies are encoded as trailers-only responses on the filter chain, | ||
|
Contributor
Author
There was a problem hiding this comment. For consistency with the above comment I should explicitly call out "but not upstream gRPC replies"
Contributor
There was a problem hiding this comment. Hm, would it work to have an ASSERT above this function where we check that if it's a gRPC reply that there's no body or trailers? Then we could move most of this comment out and say // As documented above, no special gRPC logic is necessary here. |
||
| // so we don't need to any additional encoding of the gRPC response here. This | ||
| // is different from the direct local reply case, where we do encode the gRPC | ||
| // reply as a headers-only response using an encoder callback. | ||
| /*encode_grpc_=*/nullptr, | ||
| [this, modify_headers](ResponseHeaderMapPtr&& headers, bool end_stream) -> void { | ||
| filter_manager_callbacks_.setResponseHeaders(std::move(headers)); | ||
| // TODO: Start encoding from the last decoder filter that saw the | ||
|
|
@@ -918,9 +925,18 @@ void FilterManager::sendDirectLocalReply( | |
| }, | ||
| [&](ResponseHeaderMap& response_headers, Code& code, std::string& body, | ||
| absl::string_view& content_type) -> void { | ||
| /* Direct local reply rewrites happen here since we are not going through the filter | ||
|
Contributor
Author
There was a problem hiding this comment. Should explicitly clarify "must happen here since..."
Contributor
There was a problem hiding this comment. just to check my understanding, local reply rewrites that currently exist don't go through the filter chain? Because local replies as a whole do AFIK
Contributor
Author
There was a problem hiding this comment. Local replies in the general case do go through the filter chain. However, in this direct reply case (e.g. on downstream timeouts where headers have already been sent), we are not going through the filter chain so we continue to call |
||
| * chain. */ | ||
| local_reply_.rewrite(filter_manager_callbacks_.requestHeaders().ptr(), response_headers, | ||
| stream_info_, code, body, content_type); | ||
| }, | ||
| [&](ResponseHeaderMap& headers, Code& code, std::string& body, | ||
| const absl::optional<Grpc::Status::GrpcStatus> grpc_status, | ||
| bool is_head_request) -> void { | ||
| /* Direct local reply gRPC encoding happens here since we are not going through the | ||
|
|
||
| * filter chain. */ | ||
| Utility::toGrpcTrailersOnlyResponse(headers, code, body, grpc_status, is_head_request); | ||
| }, | ||
| [&](ResponseHeaderMapPtr&& response_headers, bool end_stream) -> void { | ||
| // Move the response headers into the FilterManager to make sure they're visible to | ||
| // access logs. | ||
|
|
@@ -1044,9 +1060,55 @@ void FilterManager::encodeHeaders(ActiveStreamEncoderFilter* filter, ResponseHea | |
| } | ||
| } | ||
|
|
||
| bool modified_end_stream = (end_stream && continue_data_entry == encoder_filters_.end()); | ||
|
|
||
| // Capture the original modified_end_stream value. We may modify it again below if we add | ||
| // a response body. | ||
| const bool original_modified_end_stream = modified_end_stream; | ||
|
|
||
| // See if we should do an upstream rewrite. For local replies, always try to do a rewrite. | ||
|
Contributor
Author
There was a problem hiding this comment. "should do a response rewrite" |
||
| // For upstream replies, only try to do a rewrite if some filter rule actually applies. | ||
| // The rewrite config supports unconditional rewrites with no filter rules. This makes sense | ||
| // for local reply rewrites (e.g. one consistent format for all responses generated at Envoy) | ||
| // but makes a lot less sense for all responses upstream. | ||
| state_.do_response_rewrite_ = | ||
| local_reply_data_ != nullptr || | ||
| local_reply_.matchesAnyMapper(filter_manager_callbacks_.requestHeaders().ptr(), | ||
| *filter_manager_callbacks_.responseHeaders(), | ||
| filter_manager_callbacks_.responseTrailers().ptr(), | ||
| stream_info_); | ||
| ENVOY_STREAM_LOG(debug, | ||
| "FilterManager::encodeHeaders: end_stream={}, modified_end_stream={}, " | ||
| "state_.do_response_rewrite_={}", | ||
| *this, end_stream, modified_end_stream, state_.do_response_rewrite_); | ||
|
|
||
| if (state_.do_response_rewrite_) { | ||
| // Try to rewrite the response. This may end up not doing any actual rewrite if this is | ||
| // a local reply and there is no matching rule. If it's not a local reply, we know there's | ||
| // a match at this point since that's a condition for state_.do_response_rewrite_ above. | ||
| rewriteResponse(); | ||
|
Contributor
Author
There was a problem hiding this comment. At this point in the code, we've already run through the filter chain. Question: should we do response rewriting before or after the filter chain? If we do it before, then the filter chain has an opportunity to see the rewritten response and make its own changes. If we do it after, then we give response rewriting the final say and the highest precedence.
Contributor
There was a problem hiding this comment. As discussed offline, if we can tell ahead of time we're doing a rewrite, I think we want to let the filters see the reply that is going to the client. If they do their own stats tracking of say "how many 500s happened" I think they want to see what will be written.
Contributor
Author
There was a problem hiding this comment. One interesting case to consider is when a filter itself returns a status (or sets a header) that would match a rewrite rule. For example, if ext authz returned 403, the user may want the same 403 rewrite rule to apply as it would have if an upstream service had returned it. Same story might reply to a sentinel header like
Member
There was a problem hiding this comment. Yeah this is a tricky one. I haven't yet thought about this a huge amount, but I think in a perfect world we would do something like the following:
I do wonder if somehow this could be built using the new matching infra that @snowp created but I haven't looked at it in detail yet. Let me know if it would be easier to chat about this in a short meeting or in a doc.
Contributor
There was a problem hiding this comment. yeah, that matched my thought, where (I think) this one qualifies as unconditional and would happen first. |
||
|
|
||
| if (buffered_response_data_) { | ||
| // If we have a rewritten response body, then modified_end_stream can no longer be true | ||
| // because we have a body now. | ||
| modified_end_stream = false; | ||
| } | ||
| } | ||
|
|
||
| state_.non_100_response_headers_encoded_ = true; | ||
| filter_manager_callbacks_.encodeHeaders(headers, modified_end_stream); | ||
|
|
||
| // Encode the rewritten response right away if the original `modified_end_stream` was true. | ||
| // If it wasn't, then we know a body will be encoded later, and we'll let that function | ||
| // take care of encoding the rewritten response. | ||
| if (state_.do_response_rewrite_ && original_modified_end_stream && buffered_response_data_) { | ||
| ENVOY_STREAM_LOG(trace, | ||
| "FilterManager::encodeData calling filter_manager_callbacks_ with {} bytes " | ||
| "from buffered_response_data and modified_end_stream={}", | ||
| *this, buffered_response_data_->length(), modified_end_stream); | ||
| filter_manager_callbacks_.encodeData(*buffered_response_data_, modified_end_stream); | ||
| } | ||
|
|
||
| maybeEndEncode(modified_end_stream); | ||
|
|
||
| if (!modified_end_stream) { | ||
|
|
@@ -1180,7 +1242,19 @@ void FilterManager::encodeData(ActiveStreamEncoderFilter* filter, Buffer::Instan | |
| } | ||
|
|
||
| const bool modified_end_stream = end_stream && trailers_added_entry == encoder_filters_.end(); | ||
| if (state_.do_response_rewrite_ && buffered_response_data_) { | ||
| // If we're doing a rewrite and modified_end_stream=true, encode the buffered_response_data_ | ||
| // that was set by rewriteResponse() earlier in encodeHeaders(). | ||
| if (modified_end_stream) { | ||
| ENVOY_STREAM_LOG(trace, | ||
| "FilterManager::encodeData calling filter_manager_callbacks_ with {} bytes " | ||
| "from buffered_response_data and modified_end_stream={}", | ||
| *this, buffered_response_data_->length(), modified_end_stream); | ||
| filter_manager_callbacks_.encodeData(*buffered_response_data_, modified_end_stream); | ||
| } | ||
| } else { | ||
| filter_manager_callbacks_.encodeData(data, modified_end_stream); | ||
| } | ||
| maybeEndEncode(modified_end_stream); | ||
|
|
||
| // If trailers were adding during encodeData we need to trigger decodeTrailers in order | ||
|
|
@@ -1225,6 +1299,58 @@ void FilterManager::maybeEndEncode(bool end_stream) { | |
| } | ||
| } | ||
|
|
||
| void FilterManager::rewriteResponse() { | ||
| auto response_headers = filter_manager_callbacks_.responseHeaders(); | ||
| ASSERT(response_headers.ptr() != nullptr); | ||
|
|
||
| std::string rewritten_body{}; | ||
| absl::string_view rewritten_content_type{}; | ||
| Http::Code rewritten_code{static_cast<Http::Code>(Utility::getResponseStatus(*response_headers))}; | ||
|
|
||
| // Start with the local reply body and text/plain content type, if we have it. | ||
| if (local_reply_data_) { | ||
| rewritten_body = local_reply_data_->body_text_; | ||
| rewritten_content_type = Headers::get().ContentTypeValues.Text; | ||
| } | ||
|
|
||
| // Get rid of any buffered response data we may have at this point. We're going to use this | ||
| // as the output parameter for a rewritten body, if any. | ||
| buffered_response_data_ = nullptr; | ||
|
|
||
| ENVOY_STREAM_LOG(trace, "rewriteResponse: calling local_reply_.rewrite with body=\"{}\", code={}", | ||
| *this, rewritten_body, rewritten_code); | ||
| const bool did_body_rewrite = | ||
| local_reply_.rewrite(filter_manager_callbacks_.requestHeaders().ptr(), *response_headers, | ||
| stream_info_, rewritten_code, rewritten_body, rewritten_content_type); | ||
| ENVOY_STREAM_LOG( | ||
| trace, | ||
| "rewriteResponse: local_reply_.rewrite returned did_body_rewrite={}, body=\"{}\", " | ||
| "content_type={}, code={}", | ||
| *this, did_body_rewrite, rewritten_body, rewritten_content_type, rewritten_code); | ||
|
|
||
| if (local_reply_data_ && local_reply_data_->is_grpc_) { | ||
| // Send a trailers-only grpc response | ||
| Utility::toGrpcTrailersOnlyResponse(*response_headers, rewritten_code, rewritten_body, | ||
| local_reply_data_->grpc_status_, | ||
| local_reply_data_->is_head_request_); | ||
| return; | ||
| } | ||
|
|
||
| if (did_body_rewrite) { | ||
| buffered_response_data_ = std::make_unique<Buffer::OwnedImpl>(rewritten_body); | ||
|
|
||
| // If we rewrote with a non-empty body, set the content length and type. | ||
| // Otherwise, remove them. | ||
| if (buffered_response_data_->length() > 0) { | ||
| response_headers->setContentLength(buffered_response_data_->length()); | ||
| response_headers->setContentType(rewritten_content_type); | ||
| } else { | ||
| response_headers->removeContentLength(); | ||
| response_headers->removeContentType(); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| bool FilterManager::processNewlyAddedMetadata() { | ||
| if (request_metadata_map_vector_ == nullptr) { | ||
| return false; | ||
|
|
||
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 feel like there's got to be a way to avoid this. If I'm wrong (and I probably am given you've probably tried =P) let's rewrite to just explain why we do this
// latch the local reply data, as it us used later in blahblah function
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 think the best explanation is that we may need some of this context if we end up doing the rewrite in encodeData (because an upstream response exists and needs to be dropped / replaced by the rewritten body, so this cannot happen in encodeHeaders alone)
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.
After reviewing again, I agree this is pretty dirty but it currently serves an important purpose. Mostly, to have the local reply code path communicate to the canonical rewrite path that the local reply is grpc, is a head request, the grpc status, local reply body etc. I think this would seem cleaner, at least on the surface, if there were one member
local_reply_context_that held both local reply data (if any) and a pointer to a local reply object that should be used for the request. This could also give us an opportunity to read per-route config and override that local reply object to the per-route variant. The alternative would be to create the FM with the per-route local reply rewriter, but I'm not sure yet which makes the most sense from a lifecycle perspective.