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.

Sign up for GitHub

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

Jump to bottom

http: expose encoded headers/trailers via callbacks #14544

Merged
merged 4 commits into from
Jan 11, 2021
Merged

http: expose encoded headers/trailers via callbacks #14544

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e913c90
http: expose encoded headers/trailers via callbacks
snowp Dec 31, 2020
c6885b9
update mocks
snowp Dec 31, 2020
fd6fbed
add mock functions
snowp Jan 2, 2021
6f436c1
Merge remote-tracking branch 'envoy/master' into filter-cb-headers
snowp Jan 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions include/envoy/http/filter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -411,6 +411,12 @@ class StreamDecoderFilterCallbacks : public virtual StreamFilterCallbacks {
*/
virtual void encode100ContinueHeaders(ResponseHeaderMapPtr&& headers) PURE;

/**
* Returns the 100-Continue headers provided to encode100ContinueHeaders. Returns absl::nullopt if
* no headers have been provided yet.
*/
virtual ResponseHeaderMapOptRef continueHeaders() const PURE;

/**
* Called with headers to be encoded, optionally indicating end of stream.
*
Expand All @@ -427,6 +433,12 @@ class StreamDecoderFilterCallbacks : public virtual StreamFilterCallbacks {
virtual void encodeHeaders(ResponseHeaderMapPtr&& headers, bool end_stream,
absl::string_view details) PURE;

/**
* Returns the headers provided to encodeHeaders. Returns absl::nullopt if no headers have been
* provided yet.
*/
virtual ResponseHeaderMapOptRef responseHeaders() const PURE;

/**
* Called with data to be encoded, optionally indicating end of stream.
* @param data supplies the data to be encoded.
Expand All @@ -440,6 +452,12 @@ class StreamDecoderFilterCallbacks : public virtual StreamFilterCallbacks {
*/
virtual void encodeTrailers(ResponseTrailerMapPtr&& trailers) PURE;

/**
* Returns the trailers provided to encodeTrailers. Returns absl::nullopt if no headers have been
* provided yet.
*/
virtual ResponseTrailerMapOptRef responseTrailers() const PURE;

/**
* Called with metadata to be encoded.
*
Expand Down
1 change: 1 addition & 0 deletions include/envoy/http/header_map.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -786,6 +786,7 @@ class ResponseTrailerMap
public CustomInlineHeaderBase<CustomInlineHeaderRegistry::Type::ResponseTrailers> {};
using ResponseTrailerMapPtr = std::unique_ptr<ResponseTrailerMap>;
using ResponseTrailerMapOptRef = OptRef<ResponseTrailerMap>;
using ResponseTrailerMapOptConstRef = OptRef<const ResponseTrailerMap>;

/**
* Convenient container type for storing Http::LowerCaseString and std::string key/value pairs.
Expand Down
4 changes: 4 additions & 0 deletions source/common/http/async_client_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
#include "envoy/upstream/load_balancer.h"
#include "envoy/upstream/upstream.h"

#include "common/common/assert.h"
#include "common/common/empty_string.h"
#include "common/common/linked_object.h"
#include "common/http/message_impl.h"
Expand Down Expand Up @@ -398,10 +399,13 @@ class AsyncStreamImpl : public AsyncClient::Stream,
// The async client won't pause if sending an Expect: 100-Continue so simply
// swallows any incoming encode100Continue.
void encode100ContinueHeaders(ResponseHeaderMapPtr&&) override {}
ResponseHeaderMapOptRef continueHeaders() const override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; }
void encodeHeaders(ResponseHeaderMapPtr&& headers, bool end_stream,
absl::string_view details) override;
ResponseHeaderMapOptRef responseHeaders() const override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm mildly concerned this could cause problems if the way we configure upstream filters applies them to async client callbacks. please keep it in mind as you go forward!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I'll keep this in mind. I think perhaps it would be a good thing in general to be provide a different set of callbacks to the async client, similar to how we'd might provide a different set of callbacks for upstream and downstream HTTP filters, so maybe that will be the way we'll handle this. That way we'd avoid having to have a whole set of noop/unavailable callbacks for the async client that only makes sense for proxy streams.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, that would be really nice. we have so many callbacks here that simply don't make sense.

void encodeData(Buffer::Instance& data, bool end_stream) override;
void encodeTrailers(ResponseTrailerMapPtr&& trailers) override;
ResponseTrailerMapOptRef responseTrailers() const override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; }
void encodeMetadata(MetadataMapPtr&&) override {}
void onDecoderFilterAboveWriteBufferHighWatermark() override { ++high_watermark_calls_; }
void onDecoderFilterBelowWriteBufferLowWatermark() override {
Expand Down
12 changes: 12 additions & 0 deletions source/common/http/filter_manager.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -368,13 +368,21 @@ void ActiveStreamDecoderFilter::encode100ContinueHeaders(ResponseHeaderMapPtr&&
}
}

ResponseHeaderMapOptRef ActiveStreamDecoderFilter::continueHeaders() const {
return parent_.filter_manager_callbacks_.continueHeaders();
}

void ActiveStreamDecoderFilter::encodeHeaders(ResponseHeaderMapPtr&& headers, bool end_stream,
absl::string_view details) {
parent_.stream_info_.setResponseCodeDetails(details);
parent_.filter_manager_callbacks_.setResponseHeaders(std::move(headers));
parent_.encodeHeaders(nullptr, *parent_.filter_manager_callbacks_.responseHeaders(), end_stream);
}

ResponseHeaderMapOptRef ActiveStreamDecoderFilter::responseHeaders() const {
return parent_.filter_manager_callbacks_.responseHeaders();
}

void ActiveStreamDecoderFilter::encodeData(Buffer::Instance& data, bool end_stream) {
parent_.encodeData(nullptr, data, end_stream,
FilterManager::FilterIterationStartState::CanStartFromCurrent);
Expand All @@ -385,6 +393,10 @@ void ActiveStreamDecoderFilter::encodeTrailers(ResponseTrailerMapPtr&& trailers)
parent_.encodeTrailers(nullptr, *parent_.filter_manager_callbacks_.responseTrailers());
}

ResponseTrailerMapOptRef ActiveStreamDecoderFilter::responseTrailers() const {
return parent_.filter_manager_callbacks_.responseTrailers();
}

void ActiveStreamDecoderFilter::encodeMetadata(MetadataMapPtr&& metadata_map_ptr) {
parent_.encodeMetadata(nullptr, std::move(metadata_map_ptr));
}
Expand Down
3 changes: 3 additions & 0 deletions source/common/http/filter_manager.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,10 +284,13 @@ struct ActiveStreamDecoderFilter : public ActiveStreamFilterBase,
const absl::optional<Grpc::Status::GrpcStatus> grpc_status,
absl::string_view details) override;
void encode100ContinueHeaders(ResponseHeaderMapPtr&& headers) override;
ResponseHeaderMapOptRef continueHeaders() const override;
void encodeHeaders(ResponseHeaderMapPtr&& headers, bool end_stream,
absl::string_view details) override;
ResponseHeaderMapOptRef responseHeaders() const override;
void encodeData(Buffer::Instance& data, bool end_stream) override;
void encodeTrailers(ResponseTrailerMapPtr&& trailers) override;
ResponseTrailerMapOptRef responseTrailers() const override;
void encodeMetadata(MetadataMapPtr&& metadata_map_ptr) override;
void onDecoderFilterAboveWriteBufferHighWatermark() override;
void onDecoderFilterBelowWriteBufferLowWatermark() override;
Expand Down
6 changes: 6 additions & 0 deletions test/mocks/http/mocks.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,12 +219,15 @@ class MockStreamDecoderFilterCallbacks : public StreamDecoderFilterCallbacks,
void encode100ContinueHeaders(ResponseHeaderMapPtr&& headers) override {
encode100ContinueHeaders_(*headers);
}
MOCK_METHOD(ResponseHeaderMapOptRef, continueHeaders, (), (const));
void encodeHeaders(ResponseHeaderMapPtr&& headers, bool end_stream,
absl::string_view details) override {
stream_info_.setResponseCodeDetails(details);
encodeHeaders_(*headers, end_stream);
}
MOCK_METHOD(ResponseHeaderMapOptRef, responseHeaders, (), (const));
void encodeTrailers(ResponseTrailerMapPtr&& trailers) override { encodeTrailers_(*trailers); }
MOCK_METHOD(ResponseTrailerMapOptRef, responseTrailers, (), (const));
void encodeMetadata(MetadataMapPtr&& metadata_map) override {
encodeMetadata_(std::move(metadata_map));
}
Expand Down Expand Up @@ -374,9 +377,12 @@ class MockStreamFilter : public StreamFilter {

// Http::MockStreamEncoderFilter
MOCK_METHOD(FilterHeadersStatus, encode100ContinueHeaders, (ResponseHeaderMap & headers));
MOCK_METHOD(ResponseHeaderMapOptRef, continueHeaders, (), (const));
MOCK_METHOD(FilterHeadersStatus, encodeHeaders, (ResponseHeaderMap & headers, bool end_stream));
MOCK_METHOD(ResponseHeaderMapOptRef, responseHeaders, (), (const));
MOCK_METHOD(FilterDataStatus, encodeData, (Buffer::Instance & data, bool end_stream));
MOCK_METHOD(FilterTrailersStatus, encodeTrailers, (ResponseTrailerMap & trailers));
MOCK_METHOD(ResponseTrailerMapOptRef, responseTrailers, (), (const));
MOCK_METHOD(FilterMetadataStatus, encodeMetadata, (MetadataMap & metadata_map));
MOCK_METHOD(void, setEncoderFilterCallbacks, (StreamEncoderFilterCallbacks & callbacks));

Expand Down