-
Notifications
You must be signed in to change notification settings - Fork 4.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fault injection: add support for setting gRPC status #10841
Changes from all commits
3990560
5a77205
7db1fb3
0b369ea
160a6fa
535fa00
e6ff272
4576e19
8d19697
0ff968f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -34,7 +34,13 @@ FaultAbortConfig::FaultAbortConfig( | |
switch (abort_config.error_type_case()) { | ||
case envoy::extensions::filters::http::fault::v3::FaultAbort::ErrorTypeCase::kHttpStatus: | ||
provider_ = | ||
std::make_unique<FixedAbortProvider>(abort_config.http_status(), abort_config.percentage()); | ||
std::make_unique<FixedAbortProvider>(static_cast<Http::Code>(abort_config.http_status()), | ||
absl::nullopt, abort_config.percentage()); | ||
break; | ||
case envoy::extensions::filters::http::fault::v3::FaultAbort::ErrorTypeCase::kGrpcStatus: | ||
provider_ = std::make_unique<FixedAbortProvider>( | ||
absl::nullopt, static_cast<Grpc::Status::GrpcStatus>(abort_config.grpc_status()), | ||
abort_config.percentage()); | ||
break; | ||
case envoy::extensions::filters::http::fault::v3::FaultAbort::ErrorTypeCase::kHeaderAbort: | ||
provider_ = std::make_unique<HeaderAbortProvider>(abort_config.percentage()); | ||
|
@@ -44,10 +50,10 @@ FaultAbortConfig::FaultAbortConfig( | |
} | ||
} | ||
|
||
absl::optional<Http::Code> FaultAbortConfig::HeaderAbortProvider::statusCode( | ||
absl::optional<Http::Code> FaultAbortConfig::HeaderAbortProvider::httpStatusCode( | ||
const Http::RequestHeaderMap* request_headers) const { | ||
absl::optional<Http::Code> ret; | ||
const auto header = request_headers->get(HeaderNames::get().AbortRequest); | ||
absl::optional<Http::Code> ret = absl::nullopt; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think that we need There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep its not required, I put that in there to make it apparent. I had to read There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's fine either way. I wouldn't worry about it unless we need another push. |
||
auto header = request_headers->get(Filters::Common::Fault::HeaderNames::get().AbortRequest); | ||
if (header == nullptr) { | ||
return ret; | ||
} | ||
|
@@ -64,6 +70,21 @@ absl::optional<Http::Code> FaultAbortConfig::HeaderAbortProvider::statusCode( | |
return ret; | ||
} | ||
|
||
absl::optional<Grpc::Status::GrpcStatus> FaultAbortConfig::HeaderAbortProvider::grpcStatusCode( | ||
const Http::RequestHeaderMap* request_headers) const { | ||
auto header = request_headers->get(Filters::Common::Fault::HeaderNames::get().AbortGrpcRequest); | ||
if (header == nullptr) { | ||
return absl::nullopt; | ||
} | ||
|
||
uint64_t code; | ||
if (!absl::SimpleAtoi(header->value().getStringView(), &code)) { | ||
return absl::nullopt; | ||
} | ||
|
||
return static_cast<Grpc::Status::GrpcStatus>(code); | ||
} | ||
|
||
FaultDelayConfig::FaultDelayConfig( | ||
const envoy::extensions::filters::common::fault::v3::FaultDelay& delay_config) { | ||
switch (delay_config.fault_delay_secifier_case()) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we have a % header here also? cc @Augustyniak
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think that we should add a support for
x-envoy-fault-abort-grpc-request-percentage
HTTP header (whose implementation would be based of howx-envoy-fault-abort-request-percentage
HTTP header is defined).Together with @buildbreaker @murki we discussed potential scenarios where having
x-envoy-fault-abort-grpc-request-percentage
HTTP header would be helpful to test client-side implementation of some of the GRPC requests that Lyft mobile applications perform. This lets me believe that the addition of such header will be useful for engineers from other companies too.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I missed one thing when I posted my initial comment here.
Initially, I thought that GRPC aborts would be applied on top of http aborts meaning something like:
x-envoy-fault-abort-request-percentage
andx-envoy-fault-abort-request
HTTP headers.x-envoy-fault-abort-grpc-request-percentage
andx-envoy-fault-abort-grpc-request
HTTP headers.and for this reason I thought that we needed the addition of
x-envoy-fault-abort-grpc-request-percentage
HTTP header.Now that I know that the implementation looks more like apply "either HTTP or GRPC abort" (only one of them) I don't see any benefits of us having
x-envoy-fault-abort-grpc-request-percentage
HTTP header. Don't get me wrong, as far as I can tell current implementation works correctly and it unlocks GRPC abort tests for mobile people at Lyft but the addition ofx-envoy-fault-abort-grpc-request-percentage
HTTP header itself doesn't add any value in here - we could continue to usex-envoy-fault-abort-request-percentage
HTTP request header and functionality-wise we wouldn't lose anything. We could keep it but I think that removal makes a little more sense since it would keep the implementation simpler.I really feel bad asking you to do this @vijayrajput1 but could we get rid of
x-envoy-fault-abort-grpc-request-percentage
HTTP header and usex-envoy-fault-abort-request-percentage
for both HTTP and GRPC aborts? My deep apologizes here - I know that the addition of this header costed you a significant amount of work in the first place so it really feels bad to ask you to revert some of your changes.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @Augustyniak , I actually had this (using one header) approach in mind earlier but then I kinda convinced myself having a matching header would make things more clear. Meaning use
x-envoy-fault-abort-grpc-request-percentage
forx-envoy-fault-abort-grpc-request
vs usex-envoy-fault-abort-request-percentage
forx-envoy-fault-abort-grpc-request
. I should have raised this point earlier. I'm okay, with either of those approaches - my take is if having a separate headerx-envoy-fault-abort-grpc-request-percentage
isn't making things anymore clear than we should use only one header i.ex-envoy-fault-abort-request-percentage
- @mattklein123 any preference ?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah agreed on using 1 header. Sorry, I didn't look very deeply at this before punting review over to @Augustyniak. Apologies also!