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

Initial #1

Merged
merged 8 commits into from
Sep 8, 2014
Merged

Initial #1

merged 8 commits into from
Sep 8, 2014

Conversation

sirupsen
Copy link
Contributor

@sirupsen sirupsen commented Sep 7, 2014

Working implementation of toxiproxy with no client library. Currently it only supports adding and removing proxies, enough functionality to simulate complete network partitions and service downtime. However, it's built with space in mind to add "toxic" to a Proxy (and thereby all its Links), which is going to be a io.ReadWriteCloser to allow control over how the bytes are sent in the Link.

A key functionality this will need before hitting development and CI is the ability to "seed" with proxies with command line arguments. I'll get to that in a later PR.

This was the first complete piece I felt I could PR, and it's already a lot to review. I've added some short comments at the top of the key structs, which hopefully makes it easier to understand. I can also walk through it in person.

This should be enough to create a Ruby client library.

@Shopify/resiliency

Review @fw42 @csfrancis since you guys are on Resiliency and have touched Go :)


var AccepTimeout = time.Second

func init() {
Copy link

Choose a reason for hiding this comment

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

can this be moved to an init in the test files themselves, seems a bit out of place to have code here just to make tests easier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@sirupsen
Copy link
Contributor Author

sirupsen commented Sep 8, 2014

@graemej please check this out as well.

}

func (link *link) pipe(src, dst net.Conn) {
_, err := io.Copy(dst, src)
Copy link

Choose a reason for hiding this comment

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

I'd log the number of bytes seems like useful debugging info

@tobi
Copy link
Member

tobi commented Sep 8, 2014

Kind of disappointed that you choose to create a separate HTTP control backend. I feel like this thing could be vastly simpler if the proxy simply allowed for magic byte prefixed packages at the beginning of a new connection to configure it.

EDIT: ah, I guess that would mean you would have to use modified client libraries, which isn't a good idea.

@camilo
Copy link

camilo commented Sep 8, 2014

@tobi that would make the proxy simpler indeed, but wouldn't we pay the price having to hack all the clients on the ruby side?

@tobi
Copy link
Member

tobi commented Sep 8, 2014

@camilo yea I just reasoned this out after sending the comment. This approach here is fine then. I still feel like it could be simpler if it would just read the proxy data from command line and we would simply fork a new toxiproxy in the background whenever needed from the ruby client lib but this is semantics.

@sirupsen elegant implementation

@sirupsen
Copy link
Contributor Author

sirupsen commented Sep 8, 2014

@tobi @camilo Yeah, that's the reason why I ended up with the HTTP interface. It makes clients stupid simple. I like the idea of magic bytes concept, but unfortunately in practise it means patching IO#write to add magic-bytes. It make things quite difficult in Go as well (or other languages), since you can't hack something like this without nasty LD_PRELOAD hacks or a wrapper library.

@camilo
Copy link

camilo commented Sep 8, 2014

@sirupsen LGTM in general a walk trough tho would be nice.

@sirupsen
Copy link
Contributor Author

sirupsen commented Sep 8, 2014

@tobi We need both (haven't implemented command-line yet), because I want toxiproxy to always be running and then you need the HTTP API for changing the state of a proxy (which it only really supports through add/delete currently). This is to avoid hacking all the clients to re-establish connections for toxiproxy-tests, which is what a short-lived toxiproxy would entail. I'm also afraid fork() would end up slowing down our test suite even more, since ideally we run a lot of all those tests—as toxiproxy would be used to test the state of the matrix (which one day will be more than 2-dimensional).

My goal with toxiproxy is to make it stupid easy within Shopify to test network resiliency with any service, be it Cardserver, Shopify or other apps in the future and I think this approach is what's going to be the least in people's way and thus the most likely for people to use actively.

@tobi
Copy link
Member

tobi commented Sep 8, 2014

Yea you are right. I forgot that the connections outlive the unit tests and
therefore you need remote control over the proxy. Thank you for walking me
through the architectural choices.

  • tobi
    CEO Shopify

On Sun, Sep 7, 2014 at 8:43 PM, Simon Eskildsen notifications@github.com
wrote:

@tobi https://github.com/tobi We need both (haven't implemented
command-line yet), because I want toxiproxy to always be running and then
you need the HTTP API for changing the state of a proxy (which it only
really supports through add/delete currently). This is to avoid hacking all
the clients to re-establish connections toxiproxy-tests, which is what a
short-lived toxiproxy would entail. I'm also afraid fork() would also
will end up slowing down our test suite even more, since ideally we run a
lot of all those tests—as toxiproxy would be used to test the state of
the matrix (which one day will be more than 2-dimensional).

My goal with toxiproxy is to make it stupid easy within Shopify to test
network resiliency with any service, be it Cardserver, Shopify or other
apps in the future and I think this approach is what's going to be the
least in people's way.


Reply to this email directly or view it on GitHub
#1 (comment).

r.HandleFunc("/proxies/{name}", server.ProxyDelete).Methods("DELETE")
http.Handle("/", r)

err := http.ListenAndServe(":8474", nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick for later: this could be a config options

@sirupsen
Copy link
Contributor Author

sirupsen commented Sep 8, 2014

I'm going to merge this for now and follow up with new PRs.

sirupsen added a commit that referenced this pull request Sep 8, 2014
@sirupsen sirupsen merged commit af7e067 into master Sep 8, 2014
@sirupsen sirupsen deleted the initial branch September 8, 2014 19:09
sirupsen pushed a commit that referenced this pull request Feb 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants