Fault injection filter

[rshriram]

This is a first take providing support for systematic failure injection using fault injection filters (issue #198 ). The filter supports two types of faults: aborts and delays. Several common failure scenarios manifest at the application layer as either delayed responses to requests or failure codes from upstream clusters.

The semantics of abort vary from protocol to protocol. With HTTP, abort is expected to return a standard HTTP error code. With gRPC, abort would return one of the generic gRPC error codes. With TCP, abort would reset the tcp connection.

Similarly, the semantics of delay is different between TCP and HTTP. For HTTP, the delay is a one time delay before propagating the request upstream. For TCP, delay would be implemented as a bandwidth restriction on the TCP pipe between the downstream request and upstream cluster. [Similar principle might apply to a streaming gRPC request.]

The fault filter treats the aborts and delays as independent events and allows the user to inject either a delay or an abort or both based on a percentage of requests. This decoupling enables the user to model an overloaded service (e.g., delay response by 5s and return a HTTP 503).

This PR is a work in progress. The current PR has been tested only with HTTP (with Front proxy example, using curl client). I would like to get some feedback on the current approach. With the current code base, it is possible to inject delays and HTTP error codes, with optional ability to restrict the faults to requests containing a specific set of headers.

Some example configuration blocks (note: fault filter should be inserted before the routing filter).
delay_enabled and abort_enabled range from 1 to 100. delay_duration is in milliseconds. abort_code corresponds to HTTP or gRPC return codes (gRPC return code has not been tested).

A simple fault filter injecting a 5s delay

 "filters": [
                {
                    "type" : "decoder",
                    "name" : "fault",
                    "config" : {
                        "delay_enabled" : 100,
                        "delay_duration" : 5000,
                    }
                },
                {
                "type": "decoder",
                "name": "router",
                "config": {}
              }
            ]

A fault filter injecting a 5s delay with 50% probability, followed by an abort (code HTTP 503) with a 50% probability

 "filters": [
                {
                    "type" : "decoder",
                    "name" : "fault",
                    "config" : {
                        "delay_enabled" : 50,
                        "delay_duration" : 5000,
                        "abort_enabled" : 50,
                        "abort_code" : 503
                    }
                },
                {
                "type": "decoder",
                "name": "router",
                "config": {}
              }
            ]

Simple delay fault with header match

 "filters": [
                {
                    "type" : "decoder",
                    "name" : "fault",
                    "config" : {
                        "delay_enabled" : 100,
                        "delay_duration" : 5000
                        "headers" : [
                            {
                               "name" : "X-Foo-Bar",
                               "value" : "Bar"
                            }
                        ]
                    }
                },
                {
                "type": "decoder",
                "name": "router",
                "config": {}
              }
            ]

TODO:

• test cases
• docs

[rshriram]

Runtime support is now available to override the configuration defaults:
fault.http.delay_enabled [<=100] overrides fault filter config delay_enabled
fault.http.delay_duration [in milliseconds. E.g., 5000] overrides config delay_duration
fault.http.abort_enabled [<=100] overrides config abort_enabled
fault.http.abort_code [http abort code] overrides config abort_code

Header matches specified in the config block cannot be overridden via runtime, at this time.

[mattklein123]

@rshriram sorry this is going to have to be rebased on current master to use the new header map API. It should be a pretty small change. I will comment on the rest of it.

[mattklein123]

nit: // comments inside functions, just one line // TODO: ...

[rshriram]

to clarify.. are you saying use only the // comments inside functions and not the multiline comment syntax?

[mattklein123]

yes (putting multi line comments inside .cc files makes it difficult to bulk comment out code, in general always prefer // comments in .cc files)

[mattklein123]

nit: newline before this line

[mattklein123]

nit: newline before this line

[mattklein123]

nit: newline before this line

[mattklein123]

instead of copying this code from the route table, can we move the code into a static helper, perhaps in ConfigUtility class, and then call from both places, just passing in the headers and the headers list.

[mattklein123]

const std::vector& fault_filter_headers

[mattklein123]

At a high level code looks good. There are a few things we might want to do later like we have a specific access log error code for fault injection (see UC, UF, etc. in access log code) but we can do that in a follow up.

Please work on adding tests (check with coverage build to make sure you have 100% coverage). You can follow along with other filter tests to see how it's done.

[louiscryan]

+cc thurston

Nomenclature nits...

probabilities should range 0.0 - 1.0, rename to percentage?
durations should specify time unit or we should adopt a unit specifier on
the value. If we use ms as the duration do we want to allow floats to cover
sub-ms delays?

For abort codes we need to be cognizant of protocol specific abort
mechanisms, the distinction between HTTP2 STREAM_REJECTED and HTTP 503 is
quite meaningful. This can be dealt with in a couple of different ways:

• Allow for a per-protocol abort specification and fall back to the most
  generic (HTTP) if none available
  abort_http : 503
  abort_http2 : "STREAM_REJECTED"
  abort_grpc : "DATA_LOSS"
• Canonicalize a set of generic named failure modes and map them to the
  protocol as appropriate (The GRPC error codes might be a good starting
  point for this)

On Tue, Nov 15, 2016 at 8:33 AM, Matt Klein [email protected]
wrote:

@mattklein123 commented on this pull request.

In source/common/http/filter/fault_filter.h
#219 (review):

• FilterHeadersStatus decodeHeaders(HeaderMap& headers, bool end_stream) override;
• FilterDataStatus decodeData(Buffer::Instance& data, bool end_stream) override;
• FilterTrailersStatus decodeTrailers(HeaderMap& trailers) override;
• void setDecoderFilterCallbacks(StreamDecoderFilterCallbacks& callbacks) override;

+private:

• void onResetStream();
• void resetInternalState();
• void postDelayInjection();
• bool matches(const Http::HeaderMap& headers) const;

• FaultFilterConfigPtr config_;
• StreamDecoderFilterCallbacks* callbacks_{};
• Event::TimerPtr delay_timer_;
  +};
  +} // Http

nit: newline before this line

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#219 (review), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AIoKPA4XXSKmrRyNhisBAqwZQDNivjZzks5q-d7ggaJpZM4Kw79j
.

[jhspaybar]

At Netflix we propagate a failure injection context that travels along in a header and identifies when to fail (for example, fail all calls from any service when calling service A), as well as a configurable delay. I see you have one form of the filter that will be applied when a header is present with a specific value, would it be possible to also read the delay in milliseconds and failure percentages out of a header? It'd be best if we could even do some logic on headers as they're filtered, but in the absence of that we could do the logic in the host language of the gRPC client and just add the headers.

[mattklein123]

re: naming and percents

Typically for time units we have been doing "_ms" postfix for ms units and "_s" postfix for second units. I think doing "_percent" postfix is fine for percent units.

Internally within Envoy we don't typically use floating point. Everything ultimately gets converted to an integer typically in the range 0-10000 before computing chance tests. I don't have a strong preference as to whether from a config perspective we support floating point. Whatever people prefer is fine with me. If we start using floating point in these cases we will need to slightly modify runtime to also pre-fill floating point values (easy).

re: abort types, per @louiscryan it makes sense to at least make the config for this future proof even if we don't do a lot with it initially. E.g.,

"config" : {
  "delay_enabled_percent" : 50,
  "delay_duration_ms" : 5000,
  "abort_enabled_percent" : 50,
  "abort_specification" : {
    "type": "http",
    "http_response_code": 503
  }
}

re: header configuration, yes, ultimately we should definitely allow configuration via trusted headers. We already do this extensively within Envoy for timeouts, retries, etc. and developers find this very easy to work with. See https://lyft.github.io/envoy/docs/configuration/http_filters/router_filter.html#http-headers. Would love to see us allow header config here also, but I would recommend we do that in a follow up.

[rshriram]

@jhspaybar We do something similar internally at IBM. The current form of this PR is coarse grained. It would affect all traffic exiting an instance or entering an instance. But its a first cut :).

My plan was to push the fault blocks into the routing section so as to allow the fault injection to be performed on a per source-destination basis.

@louiscryan @mattklein123 will look into the error codes.

[louiscryan]

On the delay specification do we care about using a distribution (uniform,
exponential) instead of a step function (fixed).

On Wed, Nov 16, 2016 at 12:40 PM, Shriram Rajagopalan <
[email protected]> wrote:

@jhspaybar https://github.com/jhspaybar We do something similar
internally at IBM. The current form of this PR is coarse grained. It would
affect all traffic exiting an instance or entering an instance. But its a
first cut :).

My plan was to push the fault blocks into the routing section so as to
allow the fault injection to be performed on a per source-destination basis.

@louiscryan https://github.com/louiscryan @mattklein123
https://github.com/mattklein123 will look into the error codes.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#219 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AIoKPAtT4UKJ2YElDOP-EXaBMjZqPrL8ks5q-2pDgaJpZM4Kw79j
.

[mattklein123]

Yeah maybe we do:

"delay_specification": {
  "type": "fixed",
  "fixed_duration_ms" : 5000
}

for now to make that future proof then we can add other things later also.

[rshriram]

Why should it be restricted to just delays?

[louiscryan]

Not sure exponential would make sense for faults which are binary decisions
whereas delay values can follow a distribution

On Wed, Nov 16, 2016 at 3:59 PM, Shriram Rajagopalan <
[email protected]> wrote:

Why should it be restricted to just delays?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#219 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AIoKPB0u-TXHQNaGOxwDRsahqxDfBuXcks5q-5jagaJpZM4Kw79j
.

[rshriram]

@louiscryan I got confused by a recent comment on delay spec:

"delay_specification": {
  "type": "fixed",
  "fixed_duration_ms" : 5000
}

and thought that instead of the the abort_percent and delay_percent fields, we would be having a distribution. Looking back at the history of the conversation, what you said makes sense and I agree that it makes no sense to have a distribution for aborts.

Just to consolidate the feedback, here is the final config format. Let me know if this looks okay ( @mattklein123 @louiscryan @jhspaybar )

"config" : {
 "headers" : {
   "name" : "value"
 }
  "abort_percent" : 100,
  "abort_specification" : {
    "tcp_reset" : "true",
    "grpc_code" :  <status_code_string>,
    "http2_code": <status_code_string>,
    "http1_code" : <status_code_int>
  },
  "delay_percent" : 100,
  "delay_specification" : {
    "distribution" : "fixed|exponential|others_in_future"
    "fixed_delay_ms" : 5000,
    "other_fields_for_exponential_distribution_in_future" : 0
}

If tcp_reset is set to true, then the connection will be abruptly terminated (irrespective of the underlying protocol). [a future addition].

The config parameters can be overridden by runtime parameters:
fault/http/abort/percent, fault/http/abort/specification/http1_code, etc.

[ where can I find the integer status codes for HTTP2 and gRPC? It would be good to avoid string matches in the fault filter]

In future iterations, we could look into adding support for obtaining the fault configuration from the request headers themselves. However, we need to identify the priority of the request header configuration over that of the runtime parameters.

For TCP (in future), we could have the abort action be a connection reset, and the delay action be some form of bandwidth throttling.

[louiscryan]

Shriram,

Some notes inline below

On Fri, Nov 18, 2016 at 8:41 AM, Shriram Rajagopalan <
[email protected]> wrote:

@louiscryan https://github.com/louiscryan I got confused by a recent
comment on delay spec:

"delay_specification": {
"type": "fixed",
"fixed_duration_ms" : 5000
}

and thought that instead of the the abort_percent and delay_percent
fields, we would be having a distribution. Looking back at the history of
the conversation, what you said makes sense and I agree that it makes no
sense to have a distribution for aborts.

Just to consolidate the feedback, here is the final config format. Let me
know if this looks okay ( @mattklein123 https://github.com/mattklein123
@louiscryan https://github.com/louiscryan @jhspaybar
https://github.com/jhspaybar )

"config" : {
"headers" : {
"name" : "value"
}
"abort_percent" : 100,
"abort_specification" : {
"tcp_reset" : "true",
"grpc_code" : <status_code_string>,

"grpc_status" doc should point to
https://github.com/grpc/grpc/blob/master/doc/statuscodes.md

"http2_code": <status_code_string>,

Should point to

https://tools.ietf.org/html/rfc7540#section-7

Maybe call it "http2_error" (terminology used in spec)

"http1_code" : <status_code_int>

I think we can call this "http_status" (no need to qualify as http1 really
as the semantic spans 1 & 2, http2_error should take precedence btw if both
are specified)

},
"delay_percent" : 100,

This is a feature of a 'fixed' delay and should move adjacent to the delay,
rename to 'fixed_duration_ms' ?

"delay_specification" : {

"distribution" : "fixed|exponential|others_in_future"
"fixed_delay_ms" : 5000,
"other_fields_for_exponential_distribution_in_future" : 0

}

If tcp_reset is set to true, then the connection will be abruptly
terminated (irrespective of the underlying protocol). [a future addition].

For TCP I'd be interested in the effects of delaying the termination of the
socket as opposed to just rejecting connections. Probably best to have a
separate discussion about TCP/UDP

The config parameters can be overridden by runtime parameters:
fault/http/abort/percent, fault/http/abort/specification/http1_code, etc.

[ where can I find the integer status codes for HTTP2 and gRPC? It would
be good to avoid string matches in the fault filter]

Links provided.
@matt - Whats your stance on allowing strings vs ints for enum values in
config. Strings are generally more readable

In future iterations, we could look into adding support for obtaining the
fault configuration from the request headers themselves. However, we need
to identify the priority of the request header configuration over that of
the runtime parameters.

For TCP (in future), we could have the abort action be a connection reset,
and the delay action be some form of bandwidth throttling.

Agreed

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#219 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AIoKPIIXkOs62hlPGnMtIoyOacxCpplRks5q_dU6gaJpZM4Kw79j
.

[rshriram]

@louiscryan with respect to strings vs int, having strings in config is definitely the way to go. I was pointing to Envoy's ability to load these parameters dynamically at runtime by reading the keys from filesystem. [e.g., fault.http.abort.grpc_status would be in /root/subdir/fault/http/abort/grpc_status ]. The value provided in config can be overridden by the values in these files.

A sample code snippet that illustrates the point (ignore the syntax errors).

const std::string grpc_status_str = runtime.snapshot().getString("fault.http.abort.grpc_status");
uint64_t grpc_status_code = someHashMap[grpc_status_str];

This can be optimized by pre-processing these strings and converting them into their integer equivalents every time a new snapshot is read from the filesystem. The conversions would be specific to every filter. This might require lot more changes in Envoy.

Am I missing something @mattklein123 ?

[mattklein123]

re: strings vs. ints, sure strings are better. Feel free to use them.

@rshriram let's try to keep this initial change as scoped as possible (I prefer to do things incrementally), so if you need to use a map in each request for right now that's fine. We can separately add a runtime snapshot update callback that can be subscribed to so that this kind of behavior can be made more efficient in the future. (We have this internally in our Go port of the runtime library).

[rshriram]

"config" : {
 "headers" : {
   "name" : "value"
 }
"abort" : {
   "abort_percent" : 100,
   "tcp_reset" : <future>,
   "grpc_status" :  <status_code_string>,
   "http2_error": <status_code_string>,
   "http_status" : <status_code_int>
},
"delay" : {
    "type" : "fixed|exponential|others_in_future",
    "fixed_delay_percent" : 100,
    "fixed_duration_ms" : 5000,
    "other_fields_for_exponential_distribution_in_future" : 0
}

[louiscryan]

LGTM

On Fri, Nov 18, 2016 at 11:20 AM, Shriram Rajagopalan <
[email protected]> wrote:

"config" : {
"headers" : {
"name" : "value"
}"abort" : {
"abort_percent" : 100,
"tcp_reset" : "true_in_future",
"grpc_status" : <status_code_string>,
"http2_error": <status_code_string>,
"http_status" : <status_code_int>
},"delay" : {
"type" : "fixed|exponential|others_in_future",
"fixed_delay_percent" : 100,
"fixed_duration_ms" : 5000,
"other_fields_for_exponential_distribution_in_future" : 0
}

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#219 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AIoKPELxzT41T81mqKGDctkRJRPZi1p-ks5q_fpqgaJpZM4Kw79j
.

[rshriram]

@mattklein123 I have addressed your comments in the recent commits (last 4) including the new config format, test cases and documentation.

Note: the current version implements a fixed delay function, and supports aborts with HTTP Status only. As we discussed earlier, support for grpc_status and http2_error could be added in future iterations.

[mattklein123]

nit: Just initialize all 4 of this in the header file by default initialization. E.g.,
uint64_t abort_percent_{};

[mattklein123]

nit: TODO:

[mattklein123]

Can we have a helper function that sends the response that is called from both here as well as decodeHeaders() so we avoid the code duplication.

[mattklein123]

@rshriram a few small comments. Overall very well done. Thanks. Can you please sign CLA (see contribution readme)

[rshriram]

@mattklein123 addressed your comments and signed the CLAs.

[mattklein123]

should go in abortWithHTTPStatus()

[mattklein123]

should go in abortWithHTTPStatus()

[rshriram]

The reason I left it out of that function was because in future, we might add abortWithGRPCStatus and abortWithHTTP2Error . Rather than duplicating the stats increment counter in all 3 functions, I thought its better to keep it here. What do you think?

[mattklein123]

I would just move it for now. We can deal with the future when it happens. :)

[rshriram]

done