Add Bidirectional Toxics #132

xthexder · 2016-09-14T20:46:05Z

This is in preparation for adding an HTTP toxic as well as enabling other protocol-aware toxics.

This PR does a few things:

Keeps track of paired links so that for example an downstream link can reference an upstream link.
Adds the idea of paired toxics, allowing for an upstream and downstream toxic to share a state object.
Hides toxics with a blank name so that internal toxics aren't visible (A bidirectional toxic actually creates 2 toxics).
Handles toxicity so that paired toxics are always in the same state
Lots of tests

xthexder · 2016-09-14T20:48:49Z

toxic_collection.go

+			return nil, ErrInvalidStream
+		}
+	} else {
+		wrapper.Stream = "both"


I'm not sure what the behaviour should be here since there's only one option for bidirectional toxics.
Right now stream can be set to anything and it will just be changed to both by the server.

xthexder · 2016-09-14T20:51:13Z

Updates to CHANGELOG.md and CREATING_TOXICS.md to come.

xthexder · 2016-09-14T21:32:39Z

I think this should be ready for review now, @sirupsen
Feel free to add some other people to review, I'm not sure who else is interested in Go + Toxiproxy right now.

The WIP http toxic is here: https://github.com/Shopify/toxiproxy/blob/http-toxic/toxics/http.go
And another example of an implemented bidirectional toxic is in the tests: https://github.com/Shopify/toxiproxy/blob/bidirectional-toxics/toxics/bidirectional_test.go#L17

sirupsen · 2016-09-20T15:17:27Z

CHANGELOG.md

@@ -1,5 +1,6 @@
 # 2.1.0 (Unreleased)

+* Add bidirectional toxics #132


Could we add a little bit more of a comment here on why this is useful? What it allows us to do?

sirupsen · 2016-09-20T15:19:01Z

toxics/toxic.go

+// PipeRequest() will oparate on the upstream, while Pipe() will operate on the downstream.
+type BidirectionalToxic interface {
+	// Defines the packet flow through an upstream ToxicStub. Operates the same as Pipe().
+	PipeRequest(*ToxicStub)


Should we call this PipeUpstream instead to keep the terminology consistent?

Yeah, probably a good idea. I was originally writing this as part of the HTTP toxic, so that's where the naming came from.

sirupsen · 2016-09-20T15:26:34Z

CREATING_TOXICS.md

+Creating a bidirectional toxic is done by implementing a second `Pipe()` function called `PipeRequest()`.
+The implementation is same as a regular toxic, and can be paired with other types such as a stateful toxic.
+
+One use case of a bidirectional toxic is to mock out the backend server entirely, which is shown below:


I think it'd be more clear in people's mind if we elaborated this example a little bit to a use-case, e.g. what if we did a simple implementation of the Redis protocol (which is super simple) that fails writes of keys directly? Something like..

case c := <-stub.Request: if c == nil { stub.Close() return } if strings.HasPrefix(string(c), "SET") { c <- []byte() # failed to write } else { c <- []byte("OK") }

I'm sure this example doesn't work, but it illustrates it. An alternative would be an HTTP server, but that may be a little bit harder to make as a concise example because the protocol is more complex and AFAIK the Go STDLIB doesn't expose the HTTP parser nicely (or maybe it does?) in which case one that sends 5xx in some cases would be cool.

I'm assuming this will work out of the box with toxicity? In which case perhaps above instead of doing anything fancy, you can just return a 500 HTTP response in the example in <toxicity> of cases.

A redis example could be interesting, I'll see if I can throw something together.
Go does expose it's HTTP request/response parsing, but it requires using io.Reader / io.Writer, so it's maybe a little complicated for an example. (You can see it here https://github.com/Shopify/toxiproxy/blob/http-toxic/toxics/http.go)

Yeah, that might be too complicated for now.

sirupsen · 2016-09-20T15:27:44Z

link.go

@@ -67,7 +68,12 @@ func (link *ToxicLink) Start(name string, source io.Reader, dest io.WriteCloser)
 	}()
 	for i, toxic := range link.toxics.chain[link.direction] {
 		if stateful, ok := toxic.Toxic.(toxics.StatefulToxic); ok {
-			link.stubs[i].State = stateful.NewState()
+			if toxic.PairedToxic == nil || link.pairedLink.stubs[toxic.PairedToxic.Index].State == nil {


I'm not a huge fan of this added complexity, but since BidirectionalToxics override the entire response I don't see an easy way out. Perhaps we should emphasize more heavily that they're fundamentally different toxics because they don't parse anything to the server...

Another approach we could take is that PipeRequest can choose whether it passes directly to the downstream toxic or to the backend, in that case the default template for toxics is just to pass the data as is to the upstream. This also allows for some new interesting toxics where you manipulate with the data that's being sent to the backend, e.g. wrong key names.

Could one interface in that case work for all use-cases? Even if it's a bit more verbose. Realistically not that many toxics will be created by third-parties for now, so I think simplifying the core of Toxiproxy for more verbose toxics (that are more powerful?) isn't a bad trade-off to make.

I guess my documentation could still use some work. All of these things are possible with the current interface.
A bidirectional toxic internally is essentially just an upstream and downstream toxic paired together, with the option of sharing a user-defined state object.
They can be completely independent and work like any other toxic, or they can do fancy things with the state to share data or even completely bypass the backend server.

I originally tried to hide this implementation detail in the docs, but maybe that wasn't such a great idea and makes things more confusing.

sirupsen · 2017-03-19T23:18:46Z

@jpittis is this something you'd be interested in reviving? Protocol aware toxics would be awesome (#50).

mmollaverdi · 2017-05-02T06:27:24Z

+1. This seems very useful.

jpittis

I just gave this a read over. These review comments are mainly for myself. Assuming you're not interested in shipping this yourself @xthexder, I was going to try to get this out.

jpittis · 2017-05-27T17:37:17Z

api.go

@@ -445,7 +445,7 @@ func apiError(resp http.ResponseWriter, err error) bool {

 type proxyToxics struct {
 	*Proxy
-	Toxics []toxics.Toxic `json:"toxics"`
+	Toxics []*toxics.ToxicWrapper `json:"toxics"`


Why did we change from Toxic to ToxicWrapper?

jpittis · 2017-05-27T17:42:31Z

link.go

+
+	// If the toxic is paired, synchronize the toxicity so they are always in the same state.
+	if toxic.PairedToxic != nil {
+		link.stubs[toxic.Index].Toxicity = link.pairedLink.stubs[toxic.PairedToxic.Index].Toxicity


If toxicity is always initialized to the paired toxic's toxicity, where is the value actually configured?

jpittis · 2017-05-27T17:49:36Z

toxic_collection.go

-				// Skip the first noop toxic, it should not be visible
-				continue
+		for _, toxic := range c.chain[dir] {
+			if len(toxic.Name) > 0 {


Maybe we should have a Hidden flag rather than relying on no name?

jpittis · 2017-05-27T17:51:00Z

toxic_collection.go

-	default:
-		return nil, ErrInvalidStream
+	if toxics.New(wrapper) == nil {
+		return nil, ErrInvalidToxicType


Is ErrInvalidToxicType the only kind of error we could get here?
I should understand how this works.

mgkeen · 2017-11-16T13:18:37Z

I was hacking together a MS SQL TDS aware toxic to cut off connections when given batch or RPC calls are sent upstream. If this ever gets anywhere then give me a nudge and i will finish it off 😄

xthexder commented Sep 14, 2016

View reviewed changes

xthexder added this to the 2.1.0 milestone Sep 14, 2016

xthexder force-pushed the bidirectional-toxics branch from ac78131 to b08f2d9 Compare September 14, 2016 21:25

sirupsen reviewed Sep 20, 2016

View reviewed changes

xthexder force-pushed the bidirectional-toxics branch 2 times, most recently from a59242a to 2032741 Compare September 20, 2016 19:26

xthexder modified the milestones: 2.2.0, 2.1.0 Dec 7, 2016

xthexder changed the base branch from master to dev December 14, 2016 19:57

jpittis reviewed May 27, 2017

View reviewed changes

Jacob Wirth added 7 commits June 5, 2018 18:15

Start adding bidirectional toxics, and http toxic

2475d8c

Finish some TODO's and add request parsing for http toxic

2989b17

Add bidirectional toxic tests

e82d648

Test Toxicity and fix updating toxicity on bidirectional toxic

2dc6830

Remove http toxic, it will be in another PR

ec964a0

Add bidirectional toxic info to the docs

c75bec4

Clean up code and rename PipeRequest to PipeUpstream

0e85ee6

xthexder force-pushed the bidirectional-toxics branch from 2032741 to 0e85ee6 Compare June 6, 2018 05:30

xthexder mentioned this pull request Feb 13, 2019

Upstream aware toxics #244

Open

jpittis mentioned this pull request Apr 25, 2019

(upstream) toxic for http headers #215

Open

dentarg mentioned this pull request Apr 22, 2020

Do not support mock data #278

Open

miry self-assigned this Sep 17, 2021

miry modified the milestones: 2.2.0, 2.3.0 Oct 22, 2021

miry removed this from the 2.3.0 milestone Dec 23, 2021

miry added this to the 2.4.0 milestone Dec 23, 2021

miry mentioned this pull request Jan 5, 2022

2.4.0 Milestone #350

Closed

8 tasks

miry mentioned this pull request Mar 3, 2022

2.5.0 Milestone #367

Closed

18 tasks

miry modified the milestones: 2.4.0, 2.5.0 Mar 3, 2022

miry mentioned this pull request Sep 4, 2022

2.6.0 Milestone #434

Closed

13 tasks

miry modified the milestones: 2.5.0, 3.0.0 Sep 11, 2022

miry removed their assignment Apr 24, 2023

miry removed this from the 3.0.0 milestone Apr 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Bidirectional Toxics #132

Add Bidirectional Toxics #132

xthexder commented Sep 14, 2016

xthexder Sep 14, 2016

xthexder commented Sep 14, 2016

xthexder commented Sep 14, 2016

sirupsen Sep 20, 2016

sirupsen Sep 20, 2016

xthexder Sep 20, 2016

sirupsen Sep 20, 2016

xthexder Sep 20, 2016

sirupsen Sep 20, 2016

sirupsen Sep 20, 2016 •

edited

Loading

xthexder Sep 20, 2016

sirupsen commented Mar 19, 2017

mmollaverdi commented May 2, 2017

jpittis left a comment

jpittis May 27, 2017

jpittis May 27, 2017

jpittis May 27, 2017

jpittis May 27, 2017

mgkeen commented Nov 16, 2017

		@@ -1,5 +1,6 @@
		# 2.1.0 (Unreleased)

		* Add bidirectional toxics #132

Add Bidirectional Toxics #132

Are you sure you want to change the base?

Add Bidirectional Toxics #132

Conversation

xthexder commented Sep 14, 2016

Choose a reason for hiding this comment

xthexder commented Sep 14, 2016

xthexder commented Sep 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sirupsen Sep 20, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sirupsen commented Mar 19, 2017

mmollaverdi commented May 2, 2017

jpittis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgkeen commented Nov 16, 2017

sirupsen Sep 20, 2016 •

edited

Loading