Add support for Thrift and HTTP client-side fault injection. #666
Conversation
TODO: Determine how to short-circuit Thrift requests. TODO: Refactor into a common shared function with func parameters.
Need to add logging, tests, and potentially special Thrift error logic.
Need to adjust server name matching either for tests or permanently. The issue with using server prefix instead of the <service>.<namespace> paradigm is that care needs to be taken to ensure the prefix doesn't match too many destinations. Blast radius can explode in that model. Prometheus exports and logging are next after that.
HiramSilvey
requested review from
fishy,
kylelemons and
pacejackson
and removed request for
a team
|
Hmmm requested feedback too soon. Fixing now that I see failures. Edit: OK, good to go. |
|
FYI I just updated to remove the single random integer implementation altogether as per https://github.com/grpc/proposal/blob/master/A33-Fault-Injection.md#evaluate-possibility-fraction. |
|
@fishy @pacejackson thanks for the reviews. I just made 1 small change based on the GRPC fault injection implementation: short circuit the injected delay if the request context is cancelled. Ref: https://github.com/grpc/grpc-go/blob/38a8b9a7057218c210a599d79f3e1d07f84d18c5/xds/internal/httpfilter/fault/fault.go#L196 I changed it to exit the fault injection middleware and continue processing as per normal. If you could take a quick look to just make sure that's OK that would be appreciated. Thanks! |
@fishy it looks like @kylelemons won't be available to review this PR until the new year -- should this be blocked until then or is it OK to proceed with the 2 approvals I have now? If there is someone else I should get a sooner review from instead, let me know. Thanks! |
💸 TL;DR
This PR introduces client-side fault injection capability to HTTP and Thrift clients. The behavior is controlled via new
X-Bp-Faultheaders, and does nothing if they aren't present, are invalid, or don't match the outgoing request.📜 Details
If clients are currently passing matching
X-Bp-Faultheaders around and they happen to be valid according to this change, then unintended fault injection could take place. This is extremely unlikely to occur given how specific the headers and their valid values are, but I wanted to call it out as technically it could affect requests unintentionally.🧪 Testing Steps / Validation
Thorough unit tests were added across the common library and protocol-specific code.
✅ Checks