-
Notifications
You must be signed in to change notification settings - Fork 5.5k
WiP: Local reply mapper #8126
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
WiP: Local reply mapper #8126
Changes from 21 commits
899ec21
6da905d
8eb6f82
9bf0e6b
fc3c545
c9a686e
4213f2e
f8df3ae
dac4695
052aeeb
1499746
8f022b6
419a59c
468308a
1cce878
1efc2e3
7c75f6c
2670d0f
955bf79
cd31254
a77c82f
20abff0
9c8d757
cc55bb4
7784130
31f3cbf
5eadeca
199dd43
0aee07a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,6 +12,7 @@ import "envoy/api/v2/rds.proto"; | |
| import "envoy/api/v2/srds.proto"; | ||
| import "envoy/config/filter/accesslog/v2/accesslog.proto"; | ||
| import "envoy/type/percent.proto"; | ||
| import "envoy/type/format.proto"; | ||
|
|
||
| import "google/protobuf/any.proto"; | ||
| import "google/protobuf/duration.proto"; | ||
|
|
@@ -23,7 +24,7 @@ import "validate/validate.proto"; | |
| // [#protodoc-title: HTTP connection manager] | ||
| // HTTP connection manager :ref:`configuration overview <config_http_conn_man>`. | ||
|
|
||
| // [#comment:next free field: 35] | ||
| // [#comment:next free field: 36] | ||
| message HttpConnectionManager { | ||
| enum CodecType { | ||
| // For every new connection, the connection manager will determine which | ||
|
|
@@ -456,6 +457,37 @@ 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 = 35; | ||
| } | ||
|
|
||
| message LocalReplyConfig { | ||
| // Configuration of list of mappers which allows to filter and change local response. | ||
| // Code iterate through mappers until first match of filter. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: "The client will iterate through mappers..` |
||
| repeated ResponseMapper mapper = 1; | ||
|
|
||
| // Allows to define custom format of local reply. | ||
| type.StringOrJson format = 2; | ||
| }; | ||
|
|
||
| message ResponseMapper { | ||
| // Filter is used to determine if the response should be changed. | ||
| envoy.config.filter.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 { | ||
| // Status code for matched response. | ||
| google.protobuf.UInt32Value status_code = 1; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can this just be
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it will be good to keep this field optional if in future someone want to add e.g: headers, body. |
||
|
|
||
| // TODO: add support for GRPC rewrite. | ||
| } | ||
|
|
||
| message Rds { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| 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"; | ||
|
|
||
| message StringOrJson { | ||
| // Allows to define custom format of returned data. | ||
| oneof format { | ||
| // Plain text format. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the final version of the PR can you flesh these docs out to cross-link to the format rules, etc.? It's not super clear how the user is supposed to fill these fields out. Thank you! /wait |
||
| string string_format = 2; | ||
|
|
||
| // Json format as dictionary. | ||
| google.protobuf.Struct json_format = 3; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -713,6 +713,7 @@ resolvers | |
| resync | ||
| ret | ||
| retriable | ||
| rewriter | ||
| rpcs | ||
| rq | ||
| rtrim | ||
|
|
||
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
LocalReplyConfigbe one-perResponseMapper? What's the tradeoff here?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
LocalReplyConfigshould be one byhttp_conn_manager, but we can define many Mappers. The tradeoff is that you can define only one format per manager. So if you want to define custom format for different response it won't be possible.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.
If we think that there might be possibly a use case here for multiple custom format, I'd just allow it in the API; the implementation can probably deal with this without too much complexity. I.e. let's be really sure that one format will be sufficient forever if we take the existing design.
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 added possibility to define format for each mapper. I think about implementation: if mapper format is defined then use mapper format else use format from config (or default if not defined)
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.
Given the way you added it, it seems we could punt this until later, if you really want to keep the top-level
reply_format. So, I'd either rollback that change or remove the top-levelreply_format; I gather you want it though to reduce verbosity if you have many rewriters?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.
Yes I thought it might be good idea to define one format at top-level and allow to define custom one for each mapper if someone want different format. I can also remove top level mapper but if someone want to change the format of all responses it will require definition of mapper without
filter/rewriter.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.
OK, given that, maybe just completely ignore the above thread and rollback the per-rewriter bits, since they can be added in a non-breaking way later additively. It'll simplify the implementation work at your end. Sorry about the confusion, the PR clarified how this would look, thanks.