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

feat: add corrupt toxic #451

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

feat: add corrupt toxic #451

wants to merge 2 commits into from

Conversation

thalesmg
Copy link

@thalesmg thalesmg commented Oct 9, 2022

Adds a toxic that flips random bits of the input stream, corrupting it.

Example run:

ͳ curl -v http://localhost:4001/api/echo -H "Content-Type: application/json" -d '{"echo": "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"}'
*   Trying 127.0.0.1:4001...
* Connected to localhost (127.0.0.1) port 4001 (#0)
> POST /api/echo HTTP/1.1
> Host: localhost:4001
> User-Agent: curl/7.85.0
> Accept: */*
> Content-Type: application/json
> Content-Length: 77
>
* Mark bundle as not supporting multiuse
< HTTP/1.1 200 OK
< cache-control: max-age=0, private, must-revalidate
< content-length: 76
< content-type: application/json; charset=utf-8
< date: Sun, 09 Oct 2022 17:55:36 GMT
< server: Cowboy
< x-request-id: Fxx4NnPZWF6J4lwAAABG
<
* Connection #0 to host localhost left intact
{"echo":"aaqaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"}

(Note that it changed one of the a's to a q. Many times it may corrupt the request such that a response is not sent back.)

Adds a toxic that flips random bits of the input stream, corrupting
it.

Example run:

```
ͳ curl -v http://localhost:4001/api/echo -H "Content-Type: application/json" -d '{"echo": "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"}'
*   Trying 127.0.0.1:4001...
* Connected to localhost (127.0.0.1) port 4001 (#0)
> POST /api/echo HTTP/1.1
> Host: localhost:4001
> User-Agent: curl/7.85.0
> Accept: */*
> Content-Type: application/json
> Content-Length: 77
>
* Mark bundle as not supporting multiuse
< HTTP/1.1 200 OK
< cache-control: max-age=0, private, must-revalidate
< content-length: 76
< content-type: application/json; charset=utf-8
< date: Sun, 09 Oct 2022 17:55:36 GMT
< server: Cowboy
< x-request-id: Fxx4NnPZWF6J4lwAAABG
<
* Connection #0 to host localhost left intact
{"echo":"aaqaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"}
```

(Note that it changed one of the `a`'s to a `q`.  Many times it may
corrupt the request such that a response is not sent back.)
@miry
Copy link
Contributor

miry commented Oct 10, 2022

@thalesmg Thank you very much for contribution. I am going to review in more details, but here is something you can mean time do:

I am going to change Toxics to use a new way to manage Toxics. Here is an example: https://github.com/Shopify/toxiproxy/blob/ca6020993945335765b620146baa68656134cd38/_examples/toxics/http_toxic.go

It is explained here: https://github.com/Shopify/toxiproxy/blob/master/CREATING_TOXICS.md#using-ioreader-and-iowriter

Also to make sure it works, can you add a e2e test to https://github.com/Shopify/toxiproxy/blob/master/scripts/test-e2e#L170

@thalesmg
Copy link
Author

@miry thanks for the pointers. I'll look at them and update the PR.

@xthexder
Copy link
Contributor

xthexder commented Oct 11, 2022

I'd like to point out, this toxic produces behavior that would never be seen in the real world, unless the server or client is actually sending corrupt data.

TCP packets include a checksum, and will be automatically dropped by the network and retransmitted by the server if corruption occurs. The result is just added latency from the client's perspective.

@miry miry added the Toxiproxy label Oct 12, 2022
@miry miry added this to the 2.6.0 milestone Oct 12, 2022
@thalesmg
Copy link
Author

@xthexder Indeed, if the corruption would be applied in the packet itself (I think like tc/netem does), then the observable effect would be just an increase in latency as the packets are dropped.

My intention with this toxic was to try to make it easier to observe strange error responses from services that typically use binary protocols, such as Kafka, Pulsar, etc. . Such responses could be the result of buggy drivers / protocol implementations, and the error responses may be hard to provoke without doing some kind of mocking in the request/response of the drivers.

Is there an interest in moving forward with this PR, under those assumptions?

@miry
Copy link
Contributor

miry commented Oct 14, 2022

@thalesmg I think it is a valid example.

I agree, it would not be useful for most of users.
In same time it is nice to have a fleet of toxics
to simulate not just TCP, but Application layer protocols.

As alternative: it is still possible to build a custom toxiproxy server with custom toxics.

I would still think the better place for new toxics in the that repo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants