Skip to content

Local reply mapper - implementation #8921

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

Closed
lukidzi wants to merge 19 commits into from
Closed

Local reply mapper - implementation #8921

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
44762f6
Added api proposition
Nov 5, 2019
2d14f71
Basic implementation with current test working
Nov 5, 2019
54f3ff4
Added tests and fixed comments from code review
Nov 11, 2019
8d5bd74
Fixed format
lukidzi Nov 11, 2019
525956e
Fixed spelling
Nov 11, 2019
2b3d7de
Merge branch 'master' into local_reply_mapper_impl
lukidzi Nov 12, 2019
b71957b
Fixed missing parameter in method after merge
Nov 12, 2019
84660a6
Added docs and more tests
Nov 17, 2019
f9d6086
Merge branch 'master' into local_reply_mapper_impl
Nov 17, 2019
3e0d621
Fix import
Nov 17, 2019
bdcaa07
Fixed format
Nov 17, 2019
be3a848
Merge branch 'master' into local_reply_mapper_impl
Nov 24, 2019
ed817a5
Nullptr bcs of header not initialized
Nov 24, 2019
b4c82b6
Fix format
Nov 25, 2019
b18f1cb
Merge branch 'master' into local_reply_mapper_impl
Nov 25, 2019
2407530
Fix format
Nov 25, 2019
f4d9a07
Fix problem with empty headers
Nov 25, 2019
613203d
Another try to fix nullptr
Nov 25, 2019
7af677d
Fix test
Nov 26, 2019
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
2 changes: 2 additions & 0 deletions api/envoy/config/filter/accesslog/v2/accesslog.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@ message AccessLog {
}
}

// [#next-major-version: In the v3 API, we should consider renaming

@htuch htuch Nov 7, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: please stick to 110 character line lengths, so this shouldn't wrap.

// it to more generic filter]
// [#next-free-field: 12]
message AccessLogFilter {
oneof filter_specifier {
Expand Down
2 changes: 2 additions & 0 deletions api/envoy/config/filter/accesslog/v3alpha/accesslog.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,8 @@ message AccessLog {
}
}

// [#next-major-version: In the v3 API, we should consider renaming
// it to more generic filter]
// [#next-free-field: 12]
message AccessLogFilter {
oneof filter_specifier {
Expand Down
33 changes: 32 additions & 1 deletion api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import "envoy/api/v2/core/protocol.proto";
import "envoy/api/v2/rds.proto";
import "envoy/api/v2/srds.proto";
import "envoy/config/filter/accesslog/v2/accesslog.proto";
import "envoy/type/format.proto";
import "envoy/type/percent.proto";

import "google/protobuf/any.proto";
Expand All @@ -23,7 +24,7 @@ import "validate/validate.proto";
// [#protodoc-title: HTTP connection manager]
// HTTP connection manager :ref:`configuration overview <config_http_conn_man>`.

// [#next-free-field: 36]
// [#next-free-field: 37]
message HttpConnectionManager {
enum CodecType {
// For every new connection, the connection manager will determine which
Expand Down Expand Up @@ -467,6 +468,36 @@ message HttpConnectionManager {
// with `prefix` match set to `/dir`. Defaults to `false`. Note that slash merging is not part of
// `HTTP spec <https://tools.ietf.org/html/rfc3986>` and is provided for convenience.
bool merge_slashes = 33;

// [#not-implemented-hide:]

@htuch htuch Nov 7, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please uncomment if you are going to land this in this PR.

// Configuration of local reply returned by Envoy. Allows to specify mappings and format of
// response.
LocalReplyConfig local_reply_config = 36;
}

message LocalReplyConfig {
// Configuration of list of mappers which allows to filter and change local response.
// The client will iterate through mappers until first match.
repeated ResponseMapper mapper = 1;

// Allows to define custom format of local reply.
// It is allowed to use :ref:`command operators <config_access_log_command_operators>`
// which will be resolved to actual data.
type.StringOrJson format = 2;

@euroelessar euroelessar Nov 8, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can the format be on per mapper basis, please?
Example: we want to return different body and/or content type depending on the generated status code.

@lukidzi lukidzi Nov 8, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

During config proposition we aggred that this might be added later. So I might add it in next PR.

@euroelessar euroelessar Nov 8, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Have read the comments for prior version of pull request. It resolves the comment so no concerns from my side here.

}

message ResponseMapper {
// Filter is used to determine if the response should be changed.
accesslog.v2.AccessLogFilter filter = 1 [(validate.rules).message = {required: true}];

// Rewriter defines which values in local reply should be changed.
ResponseRewriter rewriter = 2 [(validate.rules).message = {required: true}];
}

// Configuration of new value for matched local response.
message ResponseRewriter {

@euroelessar euroelessar Nov 8, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should there be an ability to override header values?
For example to set content-type or some application-specific chillout header.

@lukidzi lukidzi Nov 8, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It might be added later.

@euroelessar euroelessar Nov 8, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Agree.

// Status code for matched response.
google.protobuf.UInt32Value status_code = 1;
}

message Rds {
Expand Down
33 changes: 32 additions & 1 deletion ...envoy/config/filter/network/http_connection_manager/v3alpha/http_connection_manager.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import "envoy/api/v3alpha/core/protocol.proto";
import "envoy/api/v3alpha/rds.proto";
import "envoy/api/v3alpha/srds.proto";
import "envoy/config/filter/accesslog/v3alpha/accesslog.proto";
import "envoy/type/v3alpha/format.proto";
import "envoy/type/v3alpha/percent.proto";

import "google/protobuf/any.proto";
Expand All @@ -23,7 +24,7 @@ import "validate/validate.proto";
// [#protodoc-title: HTTP connection manager]
// HTTP connection manager :ref:`configuration overview <config_http_conn_man>`.

// [#next-free-field: 36]
// [#next-free-field: 37]
message HttpConnectionManager {
enum CodecType {
// For every new connection, the connection manager will determine which
Expand Down Expand Up @@ -454,6 +455,36 @@ message HttpConnectionManager {
// with `prefix` match set to `/dir`. Defaults to `false`. Note that slash merging is not part of
// `HTTP spec <https://tools.ietf.org/html/rfc3986>` and is provided for convenience.
bool merge_slashes = 33;

// [#not-implemented-hide:]
// Configuration of local reply returned by Envoy. Allows to specify mappings and format of
// response.
LocalReplyConfig local_reply_config = 36;
}

message LocalReplyConfig {
// Configuration of list of mappers which allows to filter and change local response.
// The client will iterate through mappers until first match.
repeated ResponseMapper mapper = 1;

// Allows to define custom format of local reply.
// It is allowed to use :ref:`command operators <config_access_log_command_operators>`
// which will be resolved to actual data.
type.v3alpha.StringOrJson format = 2;
}

message ResponseMapper {
// Filter is used to determine if the response should be changed.
accesslog.v3alpha.AccessLogFilter filter = 1 [(validate.rules).message = {required: true}];

// Rewriter defines which values in local reply should be changed.
ResponseRewriter rewriter = 2 [(validate.rules).message = {required: true}];
}

// Configuration of new value for matched local response.
message ResponseRewriter {
// Status code for matched response.
google.protobuf.UInt32Value status_code = 1;
}

message Rds {
Expand Down
43 changes: 43 additions & 0 deletions api/envoy/type/format.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
syntax = "proto3";

package envoy.type;

option java_outer_classname = "FormatProto";
option java_multiple_files = true;
option java_package = "io.envoyproxy.envoy.type";

import "google/protobuf/struct.proto";

// [#protodoc-title: Format]

// Allows to define one of format: flat plain text or structured json.
message StringOrJson {
oneof format {
// Allows to specify flat plain text format.
//
// .. code-block:: yaml
//
// string_format: "My custom message"
//
string string_format = 1;

@euroelessar euroelessar Nov 8, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should this field be DataSource to allow fetching body from the control plane/disk?

@euroelessar euroelessar Nov 8, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actually, it looks like string_format assumes formatting.
In this case my ask is to add 3rd option with non-formattable DataSource.

@htuch htuch Nov 8, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I guess one reasonable question is "which API fields in general should be data sources?". Do we have a governing principle? Is it basically anything you might want to load from a disk? Do you have a concrete needs for this @euroelessar?

@euroelessar euroelessar Nov 8, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do you have a concrete needs for this @euroelessar?

We want an ability to return custom hard-coded user-facing html page on errors generated by envoy.
In addition to that we want to be able to serve compile-time compressed version of it for clients which support gzip/brotli.

Combination of this two factors implies that we need an ability to return potentially large binary body.
Adding this bodies to the config itself is potentially non-practical, therefore the ask for DataSource.

To clarify, I'm fine with it not being part of this particular pull request, we can contribute the necessary pieces as follow ups, assuming the API can be extended to this needs.

@htuch htuch Nov 8, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think it would be nice to get the API clean here, what you're asking for isn't unreasonable. The actual code to consume from data source is a 1 liner, so if folks are in agreement let's do this.


// Allows to specify structured data as json.
//
// .. code-block:: yaml
//
// json_format:
// protocol: "HTTP/1.1"
// message: "My message"
//
// The following JSON object would be created:
//
// .. code-block:: json
//
// {
// "protocol": "HTTP/1.1",
// "message": "My message"
// }
//
google.protobuf.Struct json_format = 2;
}
}
43 changes: 43 additions & 0 deletions api/envoy/type/v3alpha/format.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
syntax = "proto3";

package envoy.type.v3alpha;

option java_outer_classname = "FormatProto";
option java_multiple_files = true;
option java_package = "io.envoyproxy.envoy.type.v3alpha";

import "google/protobuf/struct.proto";

// [#protodoc-title: Format]

// Allows to define one of format: flat plain text or structured json.
message StringOrJson {
oneof format {
// Allows to specify flat plain text format.
//
// .. code-block:: yaml
//
// string_format: "My custom message"
//
string string_format = 1;

// Allows to specify structured data as json.
//
// .. code-block:: yaml
//
// json_format:
// protocol: "HTTP/1.1"
// message: "My message"
//
// The following JSON object would be created:
//
// .. code-block:: json
//
// {
// "protocol": "HTTP/1.1",
// "message": "My message"
// }
//
google.protobuf.Struct json_format = 2;
}
}
1 change: 1 addition & 0 deletions docs/root/api-v2/types/types.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ Types
:glob:
:maxdepth: 2

../type/format.proto
../type/hash_policy.proto
../type/http.proto
../type/http_status.proto
Expand Down
2 changes: 2 additions & 0 deletions docs/root/configuration/observability/access_log.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,8 @@ Format dictionaries have the following restrictions:

* The dictionary must map strings to strings (specifically, strings to command operators). Nesting is not currently supported.

.. _config_access_log_command_operators:

Command Operators
-----------------

Expand Down
6 changes: 4 additions & 2 deletions include/envoy/access_log/access_log.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,8 @@ class Formatter {
virtual std::string format(const Http::HeaderMap& request_headers,
const Http::HeaderMap& response_headers,
const Http::HeaderMap& response_trailers,
const StreamInfo::StreamInfo& stream_info) const PURE;
const StreamInfo::StreamInfo& stream_info,
const absl::string_view& response_body) const PURE;
};

using FormatterPtr = std::unique_ptr<Formatter>;
Expand All @@ -138,7 +139,8 @@ class FormatterProvider {
virtual std::string format(const Http::HeaderMap& request_headers,
const Http::HeaderMap& response_headers,
const Http::HeaderMap& response_trailers,
const StreamInfo::StreamInfo& stream_info) const PURE;
const StreamInfo::StreamInfo& stream_info,
const absl::string_view& response_body) const PURE;
};

using FormatterProviderPtr = std::unique_ptr<FormatterProvider>;
Expand Down
50 changes: 35 additions & 15 deletions source/common/access_log/access_log_formatter.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,12 +104,14 @@ FormatterImpl::FormatterImpl(const std::string& format) {
std::string FormatterImpl::format(const Http::HeaderMap& request_headers,
const Http::HeaderMap& response_headers,
const Http::HeaderMap& response_trailers,
const StreamInfo::StreamInfo& stream_info) const {
const StreamInfo::StreamInfo& stream_info,
const absl::string_view& response_body) const {
std::string log_line;
log_line.reserve(256);

for (const FormatterProviderPtr& provider : providers_) {
log_line += provider->format(request_headers, response_headers, response_trailers, stream_info);
log_line += provider->format(request_headers, response_headers, response_trailers, stream_info,
response_body);
}

return log_line;
Expand All @@ -125,8 +127,10 @@ JsonFormatterImpl::JsonFormatterImpl(std::unordered_map<std::string, std::string
std::string JsonFormatterImpl::format(const Http::HeaderMap& request_headers,
const Http::HeaderMap& response_headers,
const Http::HeaderMap& response_trailers,
const StreamInfo::StreamInfo& stream_info) const {
const auto output_map = toMap(request_headers, response_headers, response_trailers, stream_info);
const StreamInfo::StreamInfo& stream_info,
const absl::string_view& body) const {

@htuch htuch Nov 8, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This only applies to locally generated responses, right? We don't have to worry about the remote having a 100MB response? What if someone specifies RESP_BODY on a regular access log formatter?

@lukidzi lukidzi Nov 8, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Body is passed only for local_reply, for access log I'm passing empty string so this shouldn't be a problem.

@htuch htuch Nov 27, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think the direct response route action also goes through sendLocalReply. So, keep in mind that potentially largeish files could be present.

const auto output_map =
toMap(request_headers, response_headers, response_trailers, stream_info, body);

ProtobufWkt::Struct output_struct;
for (const auto& pair : output_map) {
Expand All @@ -147,11 +151,12 @@ std::string JsonFormatterImpl::format(const Http::HeaderMap& request_headers,

std::unordered_map<std::string, std::string> JsonFormatterImpl::toMap(
const Http::HeaderMap& request_headers, const Http::HeaderMap& response_headers,
const Http::HeaderMap& response_trailers, const StreamInfo::StreamInfo& stream_info) const {
const Http::HeaderMap& response_trailers, const StreamInfo::StreamInfo& stream_info,
const absl::string_view& body) const {
std::unordered_map<std::string, std::string> output;
for (const auto& pair : json_output_format_) {
output.emplace(pair.first, pair.second->format(request_headers, response_headers,
response_trailers, stream_info));
response_trailers, stream_info, body));
}
return output;
}
Expand Down Expand Up @@ -271,6 +276,8 @@ std::vector<FormatterProviderPtr> AccessLogFormatParser::parse(const std::string

formatters.emplace_back(FormatterProviderPtr{
new ResponseTrailerFormatter(main_header, alternative_header, max_length)});
} else if (absl::StartsWith(token, "RESP_BODY")) {
formatters.emplace_back(FormatterProviderPtr{new BodyFormatter()});
} else if (absl::StartsWith(token, DYNAMIC_META_TOKEN)) {
std::string filter_namespace;
absl::optional<size_t> max_length;
Expand Down Expand Up @@ -505,18 +512,27 @@ StreamInfoFormatter::StreamInfoFormatter(const std::string& field_name) {

std::string StreamInfoFormatter::format(const Http::HeaderMap&, const Http::HeaderMap&,
const Http::HeaderMap&,
const StreamInfo::StreamInfo& stream_info) const {
const StreamInfo::StreamInfo& stream_info,
const absl::string_view&) const {
return field_extractor_(stream_info);
}

PlainStringFormatter::PlainStringFormatter(const std::string& str) : str_(str) {}

std::string PlainStringFormatter::format(const Http::HeaderMap&, const Http::HeaderMap&,
const Http::HeaderMap&,
const StreamInfo::StreamInfo&) const {
const Http::HeaderMap&, const StreamInfo::StreamInfo&,
const absl::string_view&) const {
return str_;
}

BodyFormatter::BodyFormatter() {}

std::string BodyFormatter::format(const Http::HeaderMap&, const Http::HeaderMap&,
const Http::HeaderMap&, const StreamInfo::StreamInfo&,
const absl::string_view& response_body) const {
return response_body.data();
}

HeaderFormatter::HeaderFormatter(const std::string& main_header,
const std::string& alternative_header,
absl::optional<size_t> max_length)
Expand Down Expand Up @@ -550,8 +566,8 @@ ResponseHeaderFormatter::ResponseHeaderFormatter(const std::string& main_header,

std::string ResponseHeaderFormatter::format(const Http::HeaderMap&,
const Http::HeaderMap& response_headers,
const Http::HeaderMap&,
const StreamInfo::StreamInfo&) const {
const Http::HeaderMap&, const StreamInfo::StreamInfo&,
const absl::string_view&) const {
return HeaderFormatter::format(response_headers);
}

Expand All @@ -562,7 +578,8 @@ RequestHeaderFormatter::RequestHeaderFormatter(const std::string& main_header,

std::string RequestHeaderFormatter::format(const Http::HeaderMap& request_headers,
const Http::HeaderMap&, const Http::HeaderMap&,
const StreamInfo::StreamInfo&) const {
const StreamInfo::StreamInfo&,
const absl::string_view&) const {
return HeaderFormatter::format(request_headers);
}

Expand All @@ -573,7 +590,8 @@ ResponseTrailerFormatter::ResponseTrailerFormatter(const std::string& main_heade

std::string ResponseTrailerFormatter::format(const Http::HeaderMap&, const Http::HeaderMap&,
const Http::HeaderMap& response_trailers,
const StreamInfo::StreamInfo&) const {
const StreamInfo::StreamInfo&,
const absl::string_view&) const {
return HeaderFormatter::format(response_trailers);
}

Expand Down Expand Up @@ -615,15 +633,17 @@ DynamicMetadataFormatter::DynamicMetadataFormatter(const std::string& filter_nam

std::string DynamicMetadataFormatter::format(const Http::HeaderMap&, const Http::HeaderMap&,
const Http::HeaderMap&,
const StreamInfo::StreamInfo& stream_info) const {
const StreamInfo::StreamInfo& stream_info,
const absl::string_view&) const {
return MetadataFormatter::format(stream_info.dynamicMetadata());
}

StartTimeFormatter::StartTimeFormatter(const std::string& format) : date_formatter_(format) {}

std::string StartTimeFormatter::format(const Http::HeaderMap&, const Http::HeaderMap&,
const Http::HeaderMap&,
const StreamInfo::StreamInfo& stream_info) const {
const StreamInfo::StreamInfo& stream_info,
const absl::string_view&) const {
if (date_formatter_.formatString().empty()) {
return AccessLogDateTimeFormatter::fromTime(stream_info.startTime());
} else {
Expand Down
Loading