Expose fault.http.abort.http_status setting via HTTP header #10294
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
Augustyniak
commented
Mar 6, 2020
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
|
@mattklein123 @htuch I think that I've addressed all of your comments. I have one question regarding tests. I've tried to add an integration unit tests to fault_filter_integration_test.cc file but every time I tried to inject an abort fault via HTTP header I ended up getting the following error while running tests: It fails on A git patch that can be used to test this: |
mattklein123
requested changes
Mar 10, 2020
There was a problem hiding this comment.
Thanks, looks great with one small nit and test request. Nice work! In terms of adding an integration test, here is an example of a test that doesn't use an upstream, it just makes a request and expects a response:
envoy/test/integration/integration_test.cc
Line 182 in 9be85fb
/wait
| return ret; | ||
| } | ||
|
|
||
| if (code >= 200 and code < 600) { |
There was a problem hiding this comment.
nit: s/and/&& (some C++ compilers don't support it and it's not widely used), also please add some out of range tests.
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
Thank you! I tried something like this but it just got stuck on // Request abort controlled via header configuration.
TEST_P(FaultIntegrationTestAllProtocols, HeaderFaultAbortConfig) {
initializeFilter(header_fault_config_);
codec_client_ = makeHttpConnection(lookupPort("http"));
auto response =
codec_client_->makeHeaderOnlyRequest(Http::TestRequestHeaderMapImpl{{":method", "GET"},
{":path", "/test/long/url"},
{":scheme", "http"},
{":authority", "host"},
{"connection", "close"},
{"x-envoy-fault-abort-request", "429"}});
response->waitForEndStream();
codec_client_->waitForDisconnect();
EXPECT_TRUE(response->complete());
EXPECT_THAT(response->headers(), Envoy::Http::HttpStatusIs("429"));
}Removal of |
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
|
I'm not sure why your test is failing, but it's probably an actual issue. I would recommend running your test with debug tracing locally to debug a bit. Try the following bazel flags: |
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
HTTP headers used in integration test: I was able to debug the test using the following command: Debugging output: I confirmed that |
|
Thanks for trying the delay integration test. I think we can probably give up on it now, but before we do that, can you show a diff of your proposed test? Where are you putting the sim time advance? I think the issue is that you are likely advancing time before the actual timer is created, so it's racing, and you have no way of actually knowing when it's safe to advance time. If we keep a gauge of active faults that are currently delayed (?) you could probably wait on this gauge being 1 and that would work, but I'm not sure if we do that. cc @jmarantz. |
htuch
suggested changes
Mar 10, 2020
| reserved 1; | ||
|
|
||
| oneof error_type { | ||
| option (validate.required) = true; | ||
|
|
||
| // HTTP status code to use to abort the HTTP request. | ||
| uint32 http_status = 2 [(validate.rules).uint32 = {lt: 600 gte: 200}]; | ||
|
|
||
| // Fault aborts are controlled via an HTTP header (if applicable). | ||
| HeaderAbort header_abort = 4; |
There was a problem hiding this comment.
I would call this header_error. The abort is still controlled by the normal mechanisms, it's just the nature of the error is expressed in the header.
There was a problem hiding this comment.
I can change it but I think that header_abort is a better name because:
header_errorcan be in theory used to inject status codes such as200or204that are not errors.header_abortfollows a naming scheme used byheader_limitandheader_delayfields.header_errorwould've been a better choice ifheader_limitwasheader_kbsandheader_delaywasheader_duration.
Thoughts?
There was a problem hiding this comment.
I think I would be inclined to stick with header_abort for the reasons that @Augustyniak outlines. @htuch WDYT?
| @@ -36,6 +36,9 @@ The fault filter has the capability to allow fault configuration to be specified | |||
| This is useful in certain scenarios in which it is desired to allow the client to specify its own | |||
| fault configuration. The currently supported header controls are: | |||
|
|
|||
| * Request abort configuration via the *x-envoy-fault-abort-request* header. The header value | |||
| should be an integer that specifies the HTTP status code to return in response to a request | |||
| and must be in the range [200, 600). | |||
There was a problem hiding this comment.
Worth pointing out that header_abort needs to be set (and including an RST link back to it).
|
Diff for the test I have problems with: You were right, my time modification logic is racing with the logic responsible for delay injection and I end up modifying time before I schedule a timer. |
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
mattklein123
approved these changes
Mar 11, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description: The partial implementation of #10254. Adding a support for http header responsible for injecting faults - aborting requests with
x-envoy-fault-abort-requestHTTP header set.Risk Level: low, new feature.
Testing: Added
Docs Changes: Added
Release Notes: Added