Conditional Toxics

[hwrdtm]

What

This PR is currently in draft stage to gauge the interest in this feature / implementation. If we like it, I will proceed to add tests before marking it as reviewable.

Closes #518.

This PR implements a new feature, Conditional Toxics. While conditions are registered at the same time as toxics, when they are provided the toxic is disabled until the condition has been matched, which turns it on.

Specifically, this PR:

• Introduce a Matcher interface. All toxic condition matchers have to implement this interface.
• Implement a HttpRequestHeaderMatcher, which implements Matcher. This concrete type is also registered in a new MatcherRegistry under the key httpRequestHeaderMatcher.
  • This matcher first attempts to parse the incoming bytes through a link as a http.Request object, then checks whether a corresponding key-value pair exists in the HTTP request header, returning a bool indicating whether this check passed or not.
• Updates the ToxicCollection struct to store toxicConditions, a 2D array of *ToxicCondition that has the dimensions as ToxicCollection.chain, always. The first element in either direction in this toxicConditions array is a nil pointer. The corresponding CRUD methods against ToxicCollection.chain (chain*Toxic methods) have been updated to update this field accordingly.
  • This array is necessary to track toxic conditions in the same order as the toxics as they are being registered / created, such that a separate goroutine can go through the conditions in the same order to attempt to match them.
• Update the ToxicCollection.StartLink method to support toxic condition matching, only for upstream directions. This is done by first using the TeeReader to write the data into a new Writer when it is being read by the main thread in link.Start, and then firing a separate goroutine that passes the underlying data structure of this new Writer to matchAllToxicConditions to attempt to match against toxic conditions. If a toxic condition has successfully matched, the corresponding ToxicWrapper that wraps this condition will have its Enabled field turned on.
  • The matchAllToxicConditions requires a lock on ToxicCollection as it is expected to be run in its own goroutine.
  • ⚠️ It is not clear if I should be calling c.chainUpdateToxic(newToxicWrapper). The current code works from manual testing from a Rust client.
• Updates ToxicWrapper to store two new fields - ToxicCondition and Enabled. Enabled is used in ToxicStub.Run such that, if Enabled is false, the toxic is disabled and the NoopToxic is used to pipe data through the stub instead.
• Introduced ToxicCondition and implemented custom unmarshalling of it that is similar to ToxicWrapper.Toxic embedded field.

Motivation

The motivation for this is explained in the #518 issue.

Expected Behavior

The expected behavior is that, by passing in the following new request payload

{
  "name": "latency_downstream",
  "type": "latency",
  "stream": "downstream",
  "toxicity": 1,
  "attributes": {
    "jitter": 0,
    "latency": 2000
  },
  "condition": {
    "matcherType": "httpRequestHeaderMatcher",
    "matcherParameters": {
      "headerKey": "x-api-key",
      "headerValueRegex": "123"
    }
  }
}

this latency toxic would be created but disabled, and only enabled when there has been an HTTP request with x-api-key header and value matching the regex 123 sent from the client to upstream. Once such a request has been sent, all subsequent requests from client to upstream will be subject to this toxic.

Design Goals

These are the design goals for this feature:

• Simple and intuitive to use from clients
  • Simple REST API changes
• Simple and intuitive developer experience (debugging, feature enhancements etc.)
  • New data structures conform to existing fields
• Minimal overhead / latency and code changes (and hence hopefully bugs) to main execution path
  • Achieved with T-shaped execution
• Easily extensible / pluggable interface
  • Achieved with Matcher interface that just take in a byte slice

Limitations

These are the current limitations of this feature:

• Conditions only applicable with upstream toxics.
  • This is because, while http.ReadRequest only takes in a bufio.Reader as input, http.ReadResponse requires taking in a http.Request as input as well. At this point, I think supporting this would introduce some new assumptions into the system and also require state storage that would complicate the system overall, and hence I am unsure how to (or whether) spec this out yet.
• Not supporting boolean combination (and/or) of conditions.

Risks

Low / Medium.

• Low because the condition matching logic is executed in a separate goroutine.
• Medium because it is not clear whether c.chainUpdateToxic needs to be called when a condition has been matched. There may be fields that needs updating / functions that need to be ran (eg. in the stubs?) but are not currently because this function isn't being called right now.

Performance

This PR should not incur any noticeable performance regressions, at least not for small chains of toxics.

• While we do acquire the lock in the matchAllToxicConditions goroutine, the only concrete Matcher available - HttpRequestHeaderMatcher runs quick.

Next Steps

Getting Alignment / Buy-In:

• Do we like this feature?
• If not, how else can we support such a use case, if at all in our interest?
• If yes, do we like this implementation / direction?
• If no, why not?

Test Coverage

• Currently no tests are written in this repository. We need to.

Update interfacing tools

• We need to update Golang client, CLI and docs to reflect this new feature.

Optimize performance

• If a condition has been matched before, don't match against it anymore.

Preventing Crashes

• We should add protection against server crashing when users are deleting conditional toxics after data has been piped to upstream. Seems to be a race with the goroutine that does matchAllToxicConditions
• Figure out if I need to be calling c.chainUpdateToxic(newToxicWrapper) if in the matched case?

Testing

The Rust client has been updated in a fork to support this feature: https://github.com/hwrdtm/toxiproxy_rust/tree/hwrdtm/conditional-toxics

There has been a new test case added to this Rust client that sends 2 HTTP requests serially, and checks for their latencies:

1. We first create a proxy and a latency toxic (2 seconds) with a conditional that matches against x-api-key HTTP request header key and value 123.
2. We init a client to send an HTTP request that matches this condition.
3. We init a server that sends a response back.
4. We check that this first request took less than 2 seconds.
5. We init a client again to send another HTTP request.
6. We init a server that sends a response back.
7. We check that this second request took more than 2 seconds.

This test case passes.