Add DirectResponseAction, a new route action type that returns a hard… by brian-pane · Pull Request #393 · envoyproxy/data-plane-api

brian-pane · 2018-01-05T02:02:37Z

…-coded HTTP response.

Signed-off-by: Brian Pane bpane@pinterest.com

…-coded HTTP response. envoyproxy/envoy#2315 Signed-off-by: Brian Pane <bpane@pinterest.com>

wora · 2018-01-05T02:45:47Z

api/rds.proto

    RedirectAction redirect = 3;
+
+    // [#not-implemented-hide:] Return an arbitrary HTTP response directly, without proxying.
+    DirectResponseAction response = 7;


Name the field as direct_response. Since it has no wire cost, readability wins.

wora · 2018-01-05T02:47:15Z

api/rds.proto

+
+  // Specifies the content of the response body. If this setting is omitted,
+  // no body is included in the generated response.
+  string body = 4;


You probably want to use a real HTTP response with headers and body.

+1. I would probably remove content_type and just put in a repeated list of headers to add. You should be able to use existing header list messages in this file.

mattklein123 · 2018-01-05T03:18:18Z

api/rds.proto

+
+  // Specifies the human-readable status to be returned if
+  // the response is encoded as HTTP/1.x.
+  string reason = 2;


Practically it's not going to be possible to actually use this. The HTTP/1.1 codec will select the reason based on the code for you and you can't set it. I would drop this.

mattklein123 · 2018-01-05T03:18:48Z

api/rds.proto

+
+  // Specifies the content of the response body. If this setting is omitted,
+  // no body is included in the generated response.
+  string body = 4;


+1. I would probably remove content_type and just put in a repeated list of headers to add. You should be able to use existing header list messages in this file.

ggreenway · 2018-01-05T17:36:22Z

api/rds.proto

+
+  // Specifies the content of the response body. If this setting is omitted,
+  // no body is included in the generated response.
+  string body = 4;


I think this should be a DataSource, so that it can either be a file on disk, or provided inline.

+1 good idea.

There's a practical problem with DataSource: its inline value is of type bytes, which will require people using JSON configs to base64-encode the value.

Do you have any objection to me extending DataSource to support a string option, to save JSON users the trouble of base64-encoding?

+1, better if we can make it use one_of, which I think is wire compatible but @htuch and @wora will need to confirm.

@mattklein123 I don't understand. DataSource is already one_of ( bytes, filepath ). What are you wanting to make one_of? I think @brian-pane was asking if he can add another field to the existing one-of so it has (bytes, filepath, string).

If I'm understanding correctly, I have no objection to that, but I don't know if there's any negative protobuf side-effects, such as other uses of DataSource having to check for another value in the one-of.

If I'm not understanding correctly, can someone clarify for me?

Yes, I'm proposing to add a string to the existing one_of.

Sorry I didn't look at DataSource, so didn't know if it already had one_of. Yes, go for it! :)

Yes, this is fine. BTW, I'm moving DataSource around to its own distinct file in #398.

I'm happy to split out that part of the PR if it helps speed along this one.

…DataSource for body Signed-off-by: Brian Pane <bpane@pinterest.com>

htuch · 2018-01-05T19:37:01Z

api/base.proto

+    string filename = 1 [(validate.rules).string.min_bytes = 1];
+
+    // Bytes inlined in the configuration.
+    bytes inline = 2 [(validate.rules).bytes.min_len = 1];


Name this inline_bytes. We don't have a backwards compatibility requirement for field names; in general we avoid churn, but it's fine to do this where it is a win.

Signed-off-by: Brian Pane <bpane@pinterest.com>

ggreenway · 2018-01-05T19:52:33Z

api/rds.proto


+message DirectResponseAction {
+  // Specifies the HTTP response status to be returned.
+  google.protobuf.UInt32Value status = 1 [(validate.rules).message.required = true];


Can this be uint32 instead of google.protobuf.UInt32Value?

Also, can we validate that it is >= 100 and < 600? (I don't know if that's even possible, but would be useful if it is)

There's a precedent for using plain uint32 and range limits here:
https://github.com/envoyproxy/data-plane-api/blob/master/api/filter/http/fault.proto#L22

@htuch is that considered a good pattern to use? (I don't know the history behind why some declarations use the simple int types and others use the google.protobuf.* types.)

This came up in the newly minted STYLE.md: https://github.com/envoyproxy/data-plane-api/blob/master/STYLE.md

"Use wrapped scalar types where there is a real need for the field to have a default value that does not match the proto3 default (0/false/""). This should not be done for fields where the proto3 defaults make sense. All things being equal, pick appropriate logic, e.g. enable vs. disable for a bool field, such that the proto3 defaults work, but only where this doesn't result in API gymnastics."

Yep, +1 to @ggreenway comment above, we've been getting better at using unwrapped scalar types where it makes sense, so go for it.

…alidator Signed-off-by: Brian Pane <bpane@pinterest.com>

mattklein123 · 2018-01-05T21:22:22Z

api/rds.proto

+  uint32 status = 1 [(validate.rules).uint32 = {gte: 100, lt: 600}];
+
+  // Specifies headers to include in the HTTP response.
+  repeated HeaderValue response_headers = 2;


It occurs to me that we already have response_headers_to_add on the route. Do we need this?

If that's sufficient I might delete this and just put a comment in on body with a ref link to the other field and see it can be used for setting headers.

Signed-off-by: Brian Pane <bpane@pinterest.com>

mattklein123

nice, thanks.

Add DirectResponseAction, a new route action type that returns a hard…

9ce8e61

…-coded HTTP response. envoyproxy/envoy#2315 Signed-off-by: Brian Pane <bpane@pinterest.com>

wora reviewed Jan 5, 2018

View reviewed changes

mattklein123 reviewed Jan 5, 2018

View reviewed changes

ggreenway requested changes Jan 5, 2018

View reviewed changes

Generalize DirectResponseAction to support arbitrary headers, plus a …

d7ccda3

…DataSource for body Signed-off-by: Brian Pane <bpane@pinterest.com>

htuch reviewed Jan 5, 2018

View reviewed changes

Update DataSource field names for consistency

4c86843

Signed-off-by: Brian Pane <bpane@pinterest.com>

ggreenway reviewed Jan 5, 2018

View reviewed changes

Use a simple integer type for the status, and add a [100,599) range v…

ebdbd8f

…alidator Signed-off-by: Brian Pane <bpane@pinterest.com>

ggreenway previously approved these changes Jan 5, 2018

View reviewed changes

htuch previously approved these changes Jan 5, 2018

View reviewed changes

mattklein123 reviewed Jan 5, 2018

View reviewed changes

Remove the headers field and link to the one in RouteConfiguration

8911835

Signed-off-by: Brian Pane <bpane@pinterest.com>

brian-pane dismissed stale reviews from htuch and ggreenway via 8911835 January 5, 2018 23:10

mattklein123 approved these changes Jan 6, 2018

View reviewed changes

mattklein123 merged commit fc64085 into envoyproxy:master Jan 6, 2018

brian-pane deleted the direct-response/2315 branch January 22, 2018 22:45

This was referenced Jan 23, 2018

Send optional entity-body with direct responses envoyproxy/envoy#2429

Merged

router: send response_headers_to_add with direct responses envoyproxy/envoy#2458

Merged

Conversation

brian-pane commented Jan 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants