Skip to content
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

[REFACTORED] Add support for Thrift and HTTP client-side fault injection. #673

Merged
merged 3 commits into from
Jan 22, 2025

Conversation

HiramSilvey
Copy link
Contributor

@HiramSilvey HiramSilvey commented Jan 16, 2025

💸 TL;DR

NOTE: This is a refactor of the approved PR #666 based on offline feedback. In addition to being a refactor, this also moves the fault injection middleware to be the final middleware for each protocol, as it is meant to simulate the upstream service being unavailable.

This PR introduces client-side fault injection capability to HTTP and Thrift clients. The behavior is controlled via new X-Bp-Fault headers, 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-Fault headers 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

  • CI tests (if present) are passing
  • Adheres to code style for repo
  • Contributor License Agreement (CLA) completed if not a Reddit employee

@HiramSilvey HiramSilvey changed the title Refactored fault injection [REFACTORED] Add support for Thrift and HTTP client-side fault injection. Jan 16, 2025
@HiramSilvey HiramSilvey marked this pull request as ready for review January 17, 2025 21:52
@HiramSilvey HiramSilvey requested a review from a team as a code owner January 17, 2025 21:52
@HiramSilvey HiramSilvey requested review from fishy, kylelemons and konradreiche and removed request for a team January 17, 2025 21:52
internal/faults/common.go Outdated Show resolved Hide resolved
internal/faults/common.go Outdated Show resolved Hide resolved
httpbp/client_middlewares.go Outdated Show resolved Hide resolved
internal/faults/common.go Outdated Show resolved Hide resolved
internal/faults/common.go Outdated Show resolved Hide resolved
internal/faults/common.go Outdated Show resolved Hide resolved
internal/faults/common.go Outdated Show resolved Hide resolved
@konradreiche konradreiche requested review from konradreiche and removed request for konradreiche January 21, 2025 18:43
Copy link
Member

@konradreiche konradreiche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, a couple more asks but nothing blocking as far as I can tell 🙂

httpbp/client_middlewares.go Show resolved Hide resolved
httpbp/faults.go Show resolved Hide resolved
internal/faults/faults.go Outdated Show resolved Hide resolved
internal/faults/faults.go Outdated Show resolved Hide resolved
@HiramSilvey HiramSilvey force-pushed the refactored-fault-injection branch from eeb64ac to 0e9d4a2 Compare January 22, 2025 19:33
@HiramSilvey HiramSilvey merged commit 3d3742c into reddit:master Jan 22, 2025
2 checks passed
@HiramSilvey HiramSilvey deleted the refactored-fault-injection branch January 22, 2025 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants