Adding missing filters (http fault, redis, mongo, tcp, rate limits, etc..) by rshriram · Pull Request #192 · envoyproxy/data-plane-api

rshriram · 2017-10-13T20:16:44Z

HTTP Fault filter, Redis filter, tcp fault filter are new.
Also added faults to Mongo filter.

Signed-off-by: Shriram Rajagopalan [email protected]

cc @htuch @danielhochman (verify if stuff in Redis looks correct).
I don't know the conventions you folks have been following. So, let me know if I missed something..

Signed-off-by: Shriram Rajagopalan <[email protected]>

rshriram · 2017-10-13T20:22:58Z

api/filter/http_fault.proto

+import "google/protobuf/wrappers.proto";
+
+// Delay specification is used to inject latency into the rpc operations.
+message Delay {


Most of this is implemented, except for the exponential faults.. It was there in Istio spec. Just thought I would add it here as well. LMK if you want me to remove it.

Either remove or add a tracking issue at https://github.com/envoyproxy/envoy for implementation side (and reference here).

rshriram · 2017-10-13T20:23:17Z

api/filter/mongo_proxy.proto

+  // applied to the following MongoDB operations: Query, Insert, GetMore,
+  // and KillCursors. Once an active delay is in progress, all incoming
+  // data up until the timer event fires will be a part of the delay.
+  Delay delay = 3;


Its already implemented.. Just needs remapping config fields in the code..

rshriram · 2017-10-13T20:25:12Z

api/filter/tcp_fault.proto

+    // connection establishment.
+    google.protobuf.Duration terminate_after_period = 2;
+  }
+}


this does not exist at all :P. Thought I would get this for free when I move the mongo fault into TCP, but then Mongo does L7 style fault (on operations, instead of raw bytes as this filter proposes). My next task is to streamline mongo fault, implement TCP fault and possibly redis fault if its easy.

And for tcp, I am signing up (currently) only for delay fault, and possibly simple abort fault. Not the fancy abort after period.

htuch · 2017-10-13T20:52:20Z

api/filter/BUILD

 api_proto_library(
    name = "mongo_proxy",
    srcs = ["mongo_proxy.proto"],
+    deps = [


deps = [":http_fault"], for singleton. Please run buildifier over this file.

htuch · 2017-10-13T20:53:22Z

api/filter/http_fault.proto

+// Delay specification is used to inject latency into the rpc operations.
+message Delay {
+  // Delay type to use (fixed|exponential|..). Currently, only fixed delay (step function) is supported.
+  string type = 1;


Why not enum?

I was going by what was defined in the configs (where type was a string). I guess Enum seems more sensible given we are doing protos.

htuch · 2017-10-13T20:53:50Z

api/filter/http_fault.proto

+import "google/protobuf/wrappers.proto";
+
+// Delay specification is used to inject latency into the rpc operations.
+message Delay {


Either remove or add a tracking issue at https://github.com/envoyproxy/envoy for implementation side (and reference here).

htuch · 2017-10-13T20:54:14Z

api/filter/http_fault.proto

+  google.protobuf.UInt32Value percent = 2;
+
+  oneof http_delay_type {
+    // Add a fixed delay before forwarding the operation upstream. Format: 1h/1m/1s/1ms. MUST be >=1ms.


There is no format for Durations..

Yup. But this was intentional. For the doc-gen.. Coz it does not tell people that Duration is just a string and that people can specify 1h/1m/1s. When we generated docs for Istio, this was one of the issues. So, I went back to all protos, and manually added this, so that people reading docs knew what values to put.

Duration is only a string in JSON/YAML form. We will eventually be generating docs for {proto3, JSON, YAML}, so this interpretation only applies to some readings. I'm surprised that docgen can't inline the documentations from Duration, which is where you would expect to find this kind of explanation.

I think a better way to put it might be "See https://developers.google.com/protocol-buffers/docs/proto3#json for the JSON/YAML Duration mapping.".

htuch · 2017-10-13T20:55:08Z

api/filter/http_fault.proto

+
+  oneof http_delay_type {
+    // Add a fixed delay before forwarding the operation upstream. Format: 1h/1m/1s/1ms. MUST be >=1ms.
+    google.protobuf.Duration fixed_delay = 2;


This has field number 2, but so does percent above. oneof fields share the same number space as the fields in the message around them.

htuch · 2017-10-13T21:00:02Z

api/filter/http_fault.proto

+  // network issues, overloaded upstream service, etc.
+  Delay delay = 1;
+
+  // Abort Http request attempts and return error codes back to downstream


Nit: s/Http/HTTP/

htuch · 2017-10-13T21:00:35Z

api/filter/http_fault.proto

+      // gRPC status code to use to abort a gRPC request. NOT IMPLEMENTED
+      string grpc_status = 2 ;
+      // HTTP2 error code used to abort a Http2 request. NOT IMPLEMENTED
+      string http2_error = 3 ;


Why aren't these error codes? Why don't HTTP/HTTP2 share a code (since they have the same status code space)?

htuch · 2017-10-13T21:01:21Z

api/filter/tcp_fault.proto

+    // percentage of connections to throttle.
+    float percent = 1;
+    // bandwidth limit in "bits" per second between downstream and Envoy
+    int64 downstream_limit_bps = 2;


Why are these signed? Why aren't they wrapped, e.g. google.protobuf.UInt32Value?

htuch · 2017-10-13T21:01:32Z

api/filter/tcp_fault.proto

+
+      // Alternatively, we could wait for a certain number of bytes to be
+      // transferred to upstream before throttling the bandwidth.
+      double throttle_after_bytes = 5;


You're waiting for a fractional byte? :)

You gots no idea on the number of eyes this got.. We looked into a time machine and decided to design for the next 4000 years of prosperity

htuch · 2017-10-13T21:02:00Z

api/filter/tcp_fault.proto

+  // established, emulating remote server crash or link failure.
+  message Terminate {
+    // percentage of established Tcp connections to be terminated/reset
+    float percent = 1;


I think you should be consistent across filters in how percentages are represented (stick with what we do in eds.proto).

Signed-off-by: Shriram Rajagopalan <[email protected]>

htuch

Looks mostly good, a few more things.

htuch · 2017-10-16T15:05:25Z

api/filter/fault.proto

+import "google/protobuf/struct.proto";
+import "google/protobuf/wrappers.proto";
+
+// Delay specification is used to inject latency into the rpc/TCP proxy operations.


Nit: s/rpc/request/ (consistency with abort below).

I used RPC explicitly coz this might be used in Mongo/Redis/TCP/HTTP. May be I should use a different term than generalizing a query as an RPC

Ok, then capitalize it RPC :)

htuch · 2017-10-16T15:09:34Z

api/filter/fault.proto

+  // Applicable only for HTTP connections.
+  oneof error_type {
+    // HTTP status code to use to abort the HTTP request.
+    int32 http_status = 2;


htuch · 2017-10-16T22:30:40Z

api/filter/fault.proto

+    // HTTP status code to use to abort the HTTP request.
+    int32 http_status = 2;
+    // gRPC status code to use to abort the gRPC request.
+    int32 grpc_status = 3;


Has this been implemented? Have you some description of how we decide to do a gRPC fault rather than an HTTP one?

This has not been implemented. I thought it would be easy to add. May be I should take it off for now..

htuch · 2017-10-16T22:32:07Z

api/filter/fault.proto

+
+  // If specified, the filter will abort requests based on the values in
+  // the object. At least abort or delay must be specified.
+  Abort abort = 2;


Is there ever a situation in which this or delay would be a repeated?

Good question. My answer would be no. The filters can be stacked (i.e. multiple fault filters), such that the matching filter will activate. Changing this into an array would require changes to the filter implementation to process a sequence of faults rather than assume that each filter is responsible for one delay + abort.

htuch · 2017-10-16T22:32:36Z

api/filter/fault.proto

+  // injection filter can be applied selectively to requests that match a
+  // set of headers specified in the fault filter config. The chances of
+  // actual fault injection further depend on the values of abort_percent
+  // and fixed_delay_percent parameters.The filter will check the request’s


Nit: s. The

htuch · 2017-10-16T22:33:40Z

api/filter/redis_proxy.proto

+    // exception to this behavior is when a connection to a backend is not
+    // yet established. In that case, the connect timeout on the cluster
+    // will govern the timeout until the connection is ready.
+    google.protobuf.Duration op_timeout = 3;


This should be numbered 1.

Signed-off-by: Shriram Rajagopalan <[email protected]>

mattklein123 · 2017-10-19T03:52:18Z

Once we define these, is someone going to go through and do the work to wire them up so that we use them?

rshriram · 2017-10-19T12:47:07Z

Yea, I will do it. I just wanted all configs to be in proto form, as it makes our v2 migration easier in Istio.

mattklein123 · 2017-10-19T22:08:33Z

OK, cool. Do you want to just add them all then? We are missing router, buffer, etc.

htuch

Nits then LGTM.

htuch · 2017-10-20T19:20:47Z

api/filter/fault.proto

+import "google/protobuf/struct.proto";
+import "google/protobuf/wrappers.proto";
+
+// Delay specification is used to inject latency into the rpc/TCP proxy operations.


Ok, then capitalize it RPC :)

htuch · 2017-10-20T19:22:08Z

api/filter/redis_proxy.proto

+    // yet established. In that case, the connect timeout on the cluster
+    // will govern the timeout until the connection is ready.
+    google.protobuf.Duration op_timeout = 1;
+  }


Nit: It's more readable if you put this above settings.

+1 (I think there are a few other instances of this)

danielhochman · 2017-10-20T20:06:40Z

api/filter/fault.proto

+  // Applicable only for HTTP connections.
+  oneof error_type {
+    // HTTP status code to use to abort the HTTP request.
+    uint32 http_status = 2;


google.protobuf.UInt32Value?

I've been in two minds on this, and have stopped objecting to unwrapped types (and in some cases doing it myself) when either: it's (1) a 100% compulsory field and we don't need defaulting behavior or (2) it's embedded under something like oneof where we can test presence/absence.

It's pretty ugly in proto IMHO that we don't just have aliases for wrapped types that express this, e.g. optional_uint32, rather than the full google.protobuf.UInt32Value.

Signed-off-by: Shriram Rajagopalan <[email protected]>

rshriram · 2017-10-23T16:25:30Z

@htuch / @mattklein123 I have added a bunch of missing filters (buffer, router, ip tagging, client_ssl auth, grpc-json transcoder). These filters have configuration. Others didn't have any configuration.

Also, the missing pieces: rate limit filter proto seems different from the ones in http/tcp. Health check filter is not defined, as I am not sure of its status in the v2 api. Apart from these, all other filters have been defined.

mattklein123 · 2017-10-23T16:47:28Z

We need all the filters that have JSON schema configs, including TCP/HTTP RL and HC. Please take a look at the JSON schemas.

Signed-off-by: Shriram Rajagopalan <[email protected]>

rshriram · 2017-10-23T17:04:56Z

Added RL, and HC.

mattklein123 · 2017-10-23T19:18:58Z

api/filter/fault.proto

+    // Fixed delay (step function).
+    FIXED = 0;
+    // Exponential delay.
+    EXPONENTIAL = 1;


Are you going to implement this soon? If not, can we just remove for now?

mattklein123 · 2017-10-23T19:20:46Z

api/filter/ip_tagging.proto

+  // x-envoy-internal is set to true. If x-envoy-internal is not set or
+  // false, a request is considered external. The filter defaults to both,
+  // and it will apply to all request types.
+  string request_type = 1;


Can we make this an enum

mattklein123 · 2017-10-23T19:22:18Z

api/filter/rate_limit.proto

@@ -2,6 +2,19 @@ syntax = "proto3";

 package envoy.api.v2.filter;


Should we put network filters in a network subdirectory and http filters in an http subdirectory? Seems like it would be cleaner? If you think that is good do you want to just move stuff around and then do the cleanup required in envoy for HTTP connection manager?

PTAL.. shuffled stuff around

I think changing package names can be considered an API breaking change. I don't think we can do this with HTTP connection manager..

if its just envoymanager using it, and may be @timperret, we could do the right thing now than later?

@htuch is it really breaking? Since we aren't using Any yet, and just Struct, it seems like this would be a compile only issue and not a wire issue? If so we should clean it up?

It's breaking in the sense that a .proto for the filter config that wasn't encoded via Struct would break I think, but OK, I don't mind relaxing this one.

mattklein123 · 2017-10-23T19:22:43Z

api/filter/redis_proxy.proto

+    // yet established. In that case, the connect timeout on the cluster
+    // will govern the timeout until the connection is ready.
+    google.protobuf.Duration op_timeout = 1;
+  }


+1 (I think there are a few other instances of this)

mattklein123 · 2017-10-23T19:23:23Z

api/filter/transcoder.proto

+// to Envoy over HTTP and get proxied to a gRPC service. The HTTP mapping
+// for the gRPC service has to be defined by custom options, defined in
+// https://cloud.google.com/service-management/reference/rpc/google.api#http
+message gRPCJSONTranscoder {


nit: This is hard to read. I would do gRPCJsonTranscoder

Signed-off-by: Shriram Rajagopalan <[email protected]>

rshriram · 2017-10-24T21:12:18Z

addressed all comments. @mattklein123 / @htuch

danielhochman · 2017-10-24T21:26:44Z

api/filter/network/redis_proxy.proto

+  ConnPoolSettings settings = 3;
+
+  // Redis connection pool settings
+  message ConnPoolSettings {


move message definition before usage?

Signed-off-by: Shriram Rajagopalan <[email protected]>

danielhochman · 2017-10-24T21:33:46Z

api/filter/http/transcoder.proto

+
+  // Control options for response json. These options are passed directly
+  // to JsonPrintOptions.
+  message PrintOptions {


same here, message before usage

htuch

LGTM

htuch · 2017-10-24T21:55:43Z

ci/ci_steps.sh


 # Lint travis file.
-travis lint .travis.yml --skip-completion-check
+#travis lint .travis.yml --skip-completion-check


Signed-off-by: Shriram Rajagopalan <[email protected]>

mattklein123 · 2017-10-24T22:07:17Z

@rshriram sorry can you quickly add the new option here: https://github.com/envoyproxy/envoy/pull/1931/files

Signed-off-by: Shriram Rajagopalan <[email protected]>

rshriram · 2017-10-24T22:30:55Z

done

Signed-off-by: Shriram Rajagopalan <[email protected]>

mattklein123 · 2017-10-24T23:12:28Z

api/filter/http/fault.proto

+//
+// * Note:* The fault injection filter must be inserted before any
+// other filter, including the router filter.
+message HTTPFault {


Can we move the common stuff in this file into a common fault.proto? Maybe just api/filter/fault.proto? It's a bit weird that we import http/fault.proto into mongo filter, and will eventually have a L4 fault filter also, etc.

mattklein123 · 2017-10-24T23:12:45Z

api/filter/README.md

-These specifications will be added in the near future. In the interim, you can still supply plain JSON configuration objects
-for these missing filters by setting the `"deprecated_v1"` field to true in the filter's configuration. For example,
+If a filter configuration is not captured in the proto specification, you can still supply plain JSON configuration objects
+for such filters by setting the `"deprecated_v1"` field to true in the filter's configuration. For example,


nit: reflow to 100 cols

mattklein123 · 2017-10-24T23:13:51Z

api/filter/http/health_check.proto

+  // Specifies whether the filter operates in pass through mode or not. REQUIRED.
+  google.protobuf.BoolValue pass_through_mode = 1;
+  // Specifies the incoming HTTP endpoint that should be considered the
+  // health check endpoint. For example /healthcheck.


required? if not what is default?

This was @htuch’s suggestion. The required term is only for internal tracking, when we have wrapper types and the field is required. It’s not a doc level indicator of required fields. I can do that but better done in separate PR as one full sweep, with no proto change.

OK that's fine we can fix this in a follow up.

mattklein123

LGTM after small comments. There are probably small issues but we can fix them when you start to implement / we can add constraints when constraints are ready.

Signed-off-by: Shriram Rajagopalan <[email protected]>

mattklein123 · 2017-10-25T00:23:33Z

@rshriram per offline discussion something here is busted with Travis. We can wait till tomorrow or just go ahead and convert to Circle which should be very fast. (You can copy envoy Circle config mostly). I can turn on Circle builds for this repo if you want to switch it.

Signed-off-by: Shriram Rajagopalan <[email protected]>

rshriram · 2017-10-25T02:18:33Z

@mattklein123 added circle CI config. We can remove travis once circle CI is running smoothly.

mattklein123 · 2017-10-25T03:07:01Z

@rshriram I turned Circle on but tests failed on mater. Can you take a look and fix in a new PR?

Shriram Rajagopalan added 2 commits October 13, 2017 16:14

Adding missing filters

566e122

Signed-off-by: Shriram Rajagopalan <[email protected]>

adding tcp fault to bazel

6aed0e5

Signed-off-by: Shriram Rajagopalan <[email protected]>

rshriram commented Oct 13, 2017

View reviewed changes

htuch reviewed Oct 13, 2017

View reviewed changes

Shriram Rajagopalan added 3 commits October 13, 2017 17:47

review feedback

ff6eeea

Signed-off-by: Shriram Rajagopalan <[email protected]>

nit

523a11a

Signed-off-by: Shriram Rajagopalan <[email protected]>

syncing with docs

a00975f

Signed-off-by: Shriram Rajagopalan <[email protected]>

rshriram force-pushed the master branch from 8af4ab8 to a00975f Compare October 16, 2017 02:43

nit

867aac0

Signed-off-by: Shriram Rajagopalan <[email protected]>

htuch reviewed Oct 16, 2017

View reviewed changes

Shriram Rajagopalan added 2 commits October 17, 2017 00:39

cleanups

a729bc1

Signed-off-by: Shriram Rajagopalan <[email protected]>

trying out a longer import

9456f27

Signed-off-by: Shriram Rajagopalan <[email protected]>

htuch reviewed Oct 20, 2017

View reviewed changes

danielhochman reviewed Oct 20, 2017

View reviewed changes

Shriram Rajagopalan added 2 commits October 23, 2017 10:54

Merge remote-tracking branch 'upstream/master'

6b7220f

Signed-off-by: Shriram Rajagopalan <[email protected]>

new filters

5e0d5c2

Signed-off-by: Shriram Rajagopalan <[email protected]>

rate limit and health check filters

3819e57

Signed-off-by: Shriram Rajagopalan <[email protected]>

rshriram changed the title ~~Adding fault filters (http, redis, mongo, tcp)~~ Adding missing filters (http fault, redis, mongo, tcp, rate limits, etc..) Oct 23, 2017

mattklein123 reviewed Oct 23, 2017

View reviewed changes

Shriram Rajagopalan added 3 commits October 23, 2017 22:13

shuffling protos

1c0144a

Signed-off-by: Shriram Rajagopalan <[email protected]>

fix BUILD

e0dd931

Signed-off-by: Shriram Rajagopalan <[email protected]>

fix BUILD 2

b867cc5

Signed-off-by: Shriram Rajagopalan <[email protected]>

Shriram Rajagopalan added 4 commits October 24, 2017 17:03

moving fault to http

81186d7

Signed-off-by: Shriram Rajagopalan <[email protected]>

stray build

95286bb

Signed-off-by: Shriram Rajagopalan <[email protected]>

beautify bazel build

7ba311c

Signed-off-by: Shriram Rajagopalan <[email protected]>

REQUIRED fields

3412880

Signed-off-by: Shriram Rajagopalan <[email protected]>

danielhochman reviewed Oct 24, 2017

View reviewed changes

Shriram Rajagopalan added 3 commits October 24, 2017 17:28

Merge remote-tracking branch 'upstream/master'

3d0bfda

Signed-off-by: Shriram Rajagopalan <[email protected]>

bad commit

f01ef4f

Signed-off-by: Shriram Rajagopalan <[email protected]>

fix

24da3eb

Signed-off-by: Shriram Rajagopalan <[email protected]>

danielhochman reviewed Oct 24, 2017

View reviewed changes

htuch approved these changes Oct 24, 2017

View reviewed changes

Shriram Rajagopalan added 3 commits October 24, 2017 17:56

fix

3abb345

Signed-off-by: Shriram Rajagopalan <[email protected]>

bad commit

c7784fb

Signed-off-by: Shriram Rajagopalan <[email protected]>

fix

77ce4e8

Signed-off-by: Shriram Rajagopalan <[email protected]>

new option to route

c3665d4

Signed-off-by: Shriram Rajagopalan <[email protected]>

fix cross package ref

74d96b6

Signed-off-by: Shriram Rajagopalan <[email protected]>

mattklein123 reviewed Oct 24, 2017

View reviewed changes

nits

c50c5e9

Signed-off-by: Shriram Rajagopalan <[email protected]>

mattklein123 approved these changes Oct 25, 2017

View reviewed changes

circle ci

69c9237

Signed-off-by: Shriram Rajagopalan <[email protected]>

mattklein123 merged commit f2c1dc9 into envoyproxy:master Oct 25, 2017

dev-rice mentioned this pull request Jul 22, 2021

Fault injection documentation is misleading about TCP envoyproxy/envoy#17448

Closed

		@@ -2,6 +2,19 @@ syntax = "proto3";

		package envoy.api.v2.filter;

Conversation

rshriram commented Oct 13, 2017

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!

htuch left a comment

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 commented Oct 19, 2017

Uh oh!

rshriram commented Oct 19, 2017

Uh oh!

mattklein123 commented Oct 19, 2017

Uh oh!

htuch left a comment

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