Control % of requests faults are applied to with HTTP headers #10724

Augustyniak · 2020-04-09T16:57:00Z

Description: Add an option to control the percentage of requests we apply fault to using HTTP headers. See #10648 for more details.
Risk Level: Low, new functionality.
Testing: Added unit tests
Docs Changes: Updated
Release Notes: Updated

Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>

source/extensions/filters/common/fault/fault_config.h

mattklein123

Thanks really nice work (as usual!). Just a few small comments. Thank you!

/wait

mattklein123 · 2020-04-10T22:42:39Z

docs/root/intro/version_history.rst

@@ -28,6 +28,7 @@ Version history
 * ext_authz: disabled the use of lowercase string matcher for headers matching in HTTP-based `ext_authz`.
  Can be reverted temporarily by setting runtime feature `envoy.reloadable_features.ext_authz_http_service_enable_case_sensitive_string_matcher` to false.
 * fault: added support for controlling abort faults with :ref:`HTTP header fault configuration <config_http_filters_fault_injection_http_header>` to the HTTP fault filter.
+* fault: added support for controlling the percentage of requests that abort, delay and response rate limits faults are applied to using :ref:`HTTP headers <config_http_filters_fault_injection_http_header>` to the HTTP fault filter.


Needs a master merge and the move to the new "current" version history file.

mattklein123 · 2020-04-10T23:03:18Z

docs/root/configuration/http/http_filters/fault_filter.rst

@@ -98,7 +125,7 @@ fault.http.abort.abort_percent
  <envoy_api_field_config.filter.http.fault.v2.HTTPFault.abort>`.

 fault.http.abort.http_status
-  HTTP status code that will be used as the  of requests that will be
+  HTTP status code that will be used as the of requests that will be


not your bug but this sentence doesn't make sense. Can you fix?

mattklein123 · 2020-04-10T23:24:22Z

source/extensions/filters/http/fault/fault_filter.cc

  const auto request_delay = fault_settings_->requestDelay();
  if (request_delay == nullptr) {
    return false;
  }

+  const auto percentage_header =


From a perf perspective it would be optimal to not do this lookup unless we are actually using a header provider (similar elsewhere), especially because right now this is going to be an O(N) scan to find the header. Can you figure out a way to only do the lookup on demand? Perhaps by passing the entire map and initializing the header provider with the header to look for?

Good idea, I updated all headers providers to work like this - we pass headers map and we do a specific header lookup only if a header provider is used. After my recent changes, there should be no performance impact related to header lookup at all if fault filter is not configured to use header faults.

One more question regarding the implementation of the percentage header providers. As it is now, we try to read a percentage header (such as x-envoy-fault-abort-request-percentage) and if it's not present or its value is invalid we fallback to the value that's specified in proto config file. Do you think that falling back to 100% would make more sense (as opposed to looking at proto config file)?

envoy/source/extensions/filters/common/fault/fault_config.cc

Lines 14 to 37 in ed6fa7f

envoy::type::v3::FractionalPercent

HeaderPercentageProvider::percentage(const Http::RequestHeaderMap* request_headers) const {

if (!request_headers) {

return percentage_;

}

const auto header = request_headers->get(header_name_);

if (header == nullptr) {

return percentage_;

}

uint64_t header_numerator;

if (!absl::SimpleAtoi(header->value().getStringView(), &header_numerator)) {

return percentage_;

}

if (header_numerator > 100) {

return percentage_;

}

envoy::type::v3::FractionalPercent header_percentage;

header_percentage.set_numerator(header_numerator);

return header_percentage;

}

Do you think that falling back to 100% would make more sense (as opposed to looking at proto config file)?

I think falling back to the config is more flexible, as the operator can configure a default of 100%, 0%, etc. if they want?

That was my thought too 👍 Leaving it as it is then.

Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>

…tp-headers Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>

Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>

mattklein123 · 2020-04-13T16:54:44Z

/azp run envoy-presubmit

azure-pipelines · 2020-04-13T16:54:53Z

Azure Pipelines successfully started running 1 pipeline(s).

Augustyniak · 2020-04-13T21:27:48Z

envoy-presubmit (Linux-x64 release) is red again (I think that it's a flaky job, not an actual issue with my PR). @mattklein123 Is there any way to re-run only this one particular job?

mattklein123 · 2020-04-13T21:30:39Z

CI is broken right now. We are trying to figure out how to fix it.

mattklein123 · 2020-04-14T00:04:40Z

Please merge master.

/wait

…tp-headers Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>

Augustyniak · 2020-04-14T00:20:35Z

I think that 44eedc7 broke some of the git pre-push checks.

Checking format for third_party/statusor/statusor.cc - PASS
Checking spelling for third_party/statusor/statusor.cc
third_party/statusor/statusor.cc:7: * Copyright 2019 Google LLC
third_party/statusor/statusor.cc:41:} // namespace internal_statusor
Checked 1 file(s) and 22 comment(s), found 2 error(s).
ERROR: spell check failed. Run 'tools/spelling/check_spelling_pedantic.py fix and/or add new words to tools/spelling/spelling_dictionary.txt'

Augustyniak · 2020-04-14T00:26:08Z

Fixing spell checking in #10763

Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>

mattklein123

Thanks, a few more comments.

/wait

mattklein123 · 2020-04-14T17:12:15Z

source/extensions/filters/common/fault/fault_config.cc

+    return percentage_;
+  }
+
+  if (header_numerator > 100) {


Sorry just noticed this. Fractional percent supports 1/100, 1/10,000, etc. Do we want to support this somehow in a header? I'm wondering if this should somehow be scaled by the default percentage that is configured? WDYT?

We could definitely use a denominator of a configured default percentage - that could work. If we do this we could also have a logic that would compute an output percentage's numerator as the minimum of two values - the value of HTTP percentage header and the value defined in default configured percentage.

Why would this additional complexity be helpful?

That would be an additional safety mechanism that would allow Envoy users to define the maximum percentage of requests a given type of fault should be applied to.

It would ensure that no change in the configuration of a default percentage results in too many faults being ingested via HTTP headers. Example when the lack of this mechanism could lead to unintended behavior is presented below.

Example

Step 1

default percentage configuration:

numerator: 100 denumerator: 1000

Effective fault injection rate is 10% since 100/1000 = 10.

HTTP headers used by clients injecting faults via HTTP headers:

`x-fault-abort-request-abort`: 404 `x-fault-abort-request-abort-percentage`: 100

Effective fault injection rate is 10% since 100/1000 = 10.

Step 2

Somebody on the server side decides to change a default percentage configuration to:

numerator: 10 denumerator: 100

From their perspective there should be no negative results of this change since the effective failure rate doesn't change (it's still equal to 10% since 100/1000 = 10/100 = 10)
The person responsible for a change doesn't update clients that use HTTP headers to ingest faults.

What happens with clients using HTTP headers to inject faults?

Without proposed min logic for the calculation of a numerator

`x-fault-abort-request-abort`: 404 `x-fault-abort-request-abort-percentage`: 100

Effective fault injection rate increases to 100% since 100/100 (new denominator of a default percentage) = 100.

With proposed min logic for the calculation of a numerator

`x-fault-abort-request-abort`: 404 `x-fault-abort-request-abort-percentage`: 100

Effective fault injection rate is still at 100% since min(100/100, 10 [Default Percentage Effective Rate]) = 100.

Let me know what you think about the proposed additional to your proposed change.

Thanks for the super detailed proposal! I agree the capping makes sense, but please make sure it's well documented.

Thanks for your suggestion! Implemented here https://github.com/envoyproxy/envoy/pull/10724/files#diff-186e4ff5610d03e9dc6f8fcace2aa4a3R26-R29.

mattklein123 · 2020-04-14T17:18:52Z

source/extensions/filters/common/fault/fault_config.cc

+  if (!request_headers) {
+    return absl::nullopt;
+  }


I don't think this can happen? Same elsehwere.

Yes, it will never happen. I added this a safety measure in case we start using this class in more places in the future. That being said, I can remove this check if this kind of defensive programming is not something we do in Envoy repo.

Yes in general we don't put in this type of defensive logic which we know we can't hit currently, so I would remove.

Just so that we are on the same page. There is one place where we call percentage method with empty headers (nullptr) in Mongo related code but I don't think that header faults are supported there so we should be fine. The code I'm talking about can be found here https://github.com/envoyproxy/envoy/pull/10724/files#diff-eb7cf1a112138011ff766f2a9dc402f8R405

Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>

…tp-headers Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>

Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>

mattklein123 · 2020-04-15T16:56:38Z

Thanks, can you check CI? You can build the docs locally with docs/build.sh if that helps.

Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>

mattklein123

Awesome, thank you!

Augustyniak added 6 commits April 8, 2020 18:28

implement abort request percentage request

fe7807c

Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>

format fixes

b422786

Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>

fix typo

11b05a9

Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>

Add support for 2 more headers

29cb061

Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>

update docs

e712337

Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>

update version history

5cdba2f

Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>

Augustyniak requested a review from alyssawilk as a code owner April 9, 2020 16:57

Augustyniak added 4 commits April 9, 2020 12:22

fix percentage call

1d964be

Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>

fix

7dae973

Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>

fix

5fddbc6

Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>

Add tests

f29803a

Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>

mattklein123 self-assigned this Apr 10, 2020

fix

b1cd646

Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>

Augustyniak commented Apr 10, 2020

View reviewed changes

source/extensions/filters/common/fault/fault_config.h Show resolved Hide resolved

Augustyniak commented Apr 10, 2020

View reviewed changes

source/extensions/filters/common/fault/fault_config.h Show resolved Hide resolved

mattklein123 requested changes Apr 10, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Apr 10, 2020

Augustyniak added 3 commits April 12, 2020 21:29

Read headers only if needed

d48022a

Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>

Merge remote-tracking branch 'upstream/master' into add-percentage-ht…

0008683

…tp-headers Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>

fix/improve doc strings

ed6fa7f

Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>

repokitteh-read-only bot removed the waiting label Apr 13, 2020

Augustyniak added 3 commits April 12, 2020 22:33

revert removal

73b5e2c

Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>

Remove legacy version_history file

9473d45

Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>

Remove accidentally commited test

40f9c58

Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>

repokitteh-read-only bot added the waiting label Apr 14, 2020

Merge remote-tracking branch 'upstream/master' into add-percentage-ht…

e65b6ca

…tp-headers Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>

repokitteh-read-only bot removed the waiting label Apr 14, 2020

🔨

e365cb1

Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>

mattklein123 requested changes Apr 14, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Apr 14, 2020

Augustyniak added 6 commits April 14, 2020 20:57

Remove defensive programming checks

1d69942

Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>

Make tests pass

4d7bdf9

Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>

Clean up initializer and properties

da2bdb5

Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>

Use the numerator of default percentage

1b81ad7

Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>

Update docs

0d3b05c

Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>

fix formatting

dbed763

Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>

repokitteh-read-only bot removed the waiting label Apr 15, 2020

Augustyniak added 2 commits April 15, 2020 09:33

Merge remote-tracking branch 'upstream/master' into add-percentage-ht…

3383e5d

…tp-headers Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>

fix order in current.rst

be0db9e

Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>

mattklein123 added the waiting label Apr 15, 2020

fix docs

1ad1368

Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>

repokitteh-read-only bot removed the waiting label Apr 15, 2020

mattklein123 approved these changes Apr 15, 2020

View reviewed changes

mattklein123 merged commit 6cbbe0d into envoyproxy:master Apr 15, 2020

Augustyniak mentioned this pull request Apr 15, 2020

Add HTTP headers for controlling percentage of requests faults should be applied to #10648

Closed

Augustyniak mentioned this pull request Apr 23, 2020

fault injection: add support for setting gRPC status #10841

Merged

lidizheng mentioned this pull request Feb 12, 2021

A33: Client-Side Fault Injection grpc/proposal#201

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Control % of requests faults are applied to with HTTP headers #10724

Control % of requests faults are applied to with HTTP headers #10724

Augustyniak commented Apr 9, 2020

mattklein123 left a comment

mattklein123 Apr 10, 2020

mattklein123 Apr 10, 2020

mattklein123 Apr 10, 2020

Augustyniak Apr 13, 2020

mattklein123 Apr 13, 2020

Augustyniak Apr 13, 2020

mattklein123 commented Apr 13, 2020

azure-pipelines bot commented Apr 13, 2020

Augustyniak commented Apr 13, 2020 •

edited

Loading

mattklein123 commented Apr 13, 2020

mattklein123 commented Apr 14, 2020

Augustyniak commented Apr 14, 2020 •

edited

Loading

Augustyniak commented Apr 14, 2020

mattklein123 left a comment

mattklein123 Apr 14, 2020

Augustyniak Apr 14, 2020

mattklein123 Apr 14, 2020

Augustyniak Apr 15, 2020

mattklein123 Apr 14, 2020

Augustyniak Apr 14, 2020

mattklein123 Apr 14, 2020

Augustyniak Apr 15, 2020

mattklein123 commented Apr 15, 2020

mattklein123 left a comment

	envoy::type::v3::FractionalPercent
	HeaderPercentageProvider::percentage(const Http::RequestHeaderMap* request_headers) const {
	if (!request_headers) {
	return percentage_;
	}

	const auto header = request_headers->get(header_name_);
	if (header == nullptr) {
	return percentage_;
	}

	uint64_t header_numerator;
	if (!absl::SimpleAtoi(header->value().getStringView(), &header_numerator)) {
	return percentage_;
	}

	if (header_numerator > 100) {
	return percentage_;
	}

	envoy::type::v3::FractionalPercent header_percentage;
	header_percentage.set_numerator(header_numerator);
	return header_percentage;
	}

Control % of requests faults are applied to with HTTP headers #10724

Control % of requests faults are applied to with HTTP headers #10724

Conversation

Augustyniak commented Apr 9, 2020

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 commented Apr 13, 2020

azure-pipelines bot commented Apr 13, 2020

Augustyniak commented Apr 13, 2020 • edited Loading

mattklein123 commented Apr 13, 2020

mattklein123 commented Apr 14, 2020

Augustyniak commented Apr 14, 2020 • edited Loading

Augustyniak commented Apr 14, 2020

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Example

Step 1

Step 2

What happens with clients using HTTP headers to inject faults?

Without proposed min logic for the calculation of a numerator

With proposed min logic for the calculation of a numerator

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 commented Apr 15, 2020

mattklein123 left a comment

Choose a reason for hiding this comment

Augustyniak commented Apr 13, 2020 •

edited

Loading

Augustyniak commented Apr 14, 2020 •

edited

Loading