Proxy subscripts

[prdoyle]

Abstract

Allows multiple proxies to be created for the same target host:port so parallel tests can test different toxics on the same container at the same time.

Rationale

Toxiproxy itself allows proxies to be assigned arbitrary names, but ToxiproxyContainer simplifies this a bit by auto-generating the names in the form host:port. This allows you to do getProxy multiple times for the same host:port and get back the same proxy. Handy.

To maintain this functionality, I added getProxy variants that allow you to supply a so-called "subscript". Then, the auto-generated name takes the form host:port:subscript. This way, you can have mutliple proxies for the same host:port but also don't need to manage name strings, or worry about having two proxies with the same name.

Use case

For my use, in unit tests, I intend to use a counter as the subscript, giving each test class its own separate proxy, so that toxics in one class won't affect the others, allowing the classes to run in parallel.

[kiview]

Hey @prdoyle, thanks for this PR.

CI is failing because of code style issues, please run ./gradlew :toxiproxy:spotlessApply to fix these violations.

[prdoyle]

Sorry, rookie mistake.

(I hadn't run spotless because prettier requires NPM which I don't have installed. I tried to just "be careful" instead but failed. 😛 )

[prdoyle]

In my latest commit, spotless seems to have changed all the line endings. 😠

Let me see if I can fix that.

[prdoyle]

Ok, after installing node.js and downgrading to Java 8, I'm able to run Spotless. It still messed up the line endings, but I've manually put them back. It's failing spotlessCheck on my laptop but I'll push it up and see if it passes in CircleCI. 🤷

FYI I'm running Windows with Cygwin, no WSL. 👴

[prdoyle]

Alrighty @kiview - looks like I'm good to go.

I'd be happy to rebase this branch to make the history cleaner if you want.

[kiview]

Woah, hardcore environment 😅 And sorry about the dependency on NPM, we tried a couple of Gradle plugins to install it locally on demand, but they were not really cross-platform compatible. Using something like GitPod or Codespaces can be a good workaround for such environments btw. (we will look into providing a GitPod configuration soon).

Rebasing is not necessary, we squash commits on merge anyway 🙂

[prdoyle]

Huh. Could the red X have been caused by me?

org.testcontainers.containers.ContainerFetchException: Can't get Docker image: RemoteDockerImage(imageName=redis:3.0.2, imagePullPolicy=DefaultPullPolicy(),

[kiview]

Unlikely, I triggered a re-run fo the failed jobs.

[prdoyle]

Hmm, I wonder if I should have used a synchronizedMap instead of ConcurrentHashMap? In case of a race condition, we probably don't want to ask Toxiproxy for two proxies with the same name. 🤔

[kiview]

@prdoyle This is a good point, also think about how suitable for parallelization the implementation is. However, for our use case, I don't think the different semantics of ConcurrentHashMap and synchronizedMap() would have an effect, since we don't need a lock on the whole map object, or?

[prdoyle]

@kiview - ConcurrentMap.computeIfAbsent can execute the given mapping function multiple times. It only guarantees that it will insert the result as if by putIfAbsent.

The difference would be:

• ConcurrentHashMap could call Toxiproxy multiple times to make multiple proxies with the same name. I haven't yet checked what Toxiproxy does in this situation. Ideally it would ignore the redundant requests.
• synchronizedMap would ensure that this class makes only one request to Toxiproxy at a time; others would block and wait.

The latter seems safer semantically, but it does make threads block.

If Toxiproxy's POST is idempotent, we're good. Otherwise, if we're using ConcurrentHashMap then we'll need to consider what happens if multiple threads try to make the same proxy at the same time.

[prdoyle]

Toxiproxy is a little light on documentation, but I tried an experiment that has me convinced ConcurrentHashMap is ok.

I tried changing computeIfAbsent to put. This caused my tests to call the proxy POST endpoint multiple times with the same name and host:port. The tests worked. I believe this indicates that Toxiproxy is idempotent.

[kiview]

Thanks for looking into it. Since the operations on the map should never be performance-critical across its use cases, synchronizedMap should also be fine. Still, given the indication of idempotence of Toxiproxy, I'm okay to keep it like this. Your call 🙂

[prdoyle]

Thanks @kiview, totally agree. I'm on the fence. Blocking does raise the specter of deadlock, so if it's not required, I think I might lean 51% toward leaving it with ConcurrentHashMap.

[prdoyle]

@bsideup - no, I didn't consider that. I'm fairly new to Toxiproxy. I can try it today and see how that feels.

I would say I don't think it's that unusual or advanced for people to want to run their tests in parallel though.

[prdoyle]

(For the record, I totally agree with @bsideup that those are the right questions to ask.)

[prdoyle]

I never quite figured out a clean way to do this just using the client directly. I can take another stab at it if you all still feel that's a worthwhile thing to try.

[eddumelendez]

it's quite similar with what was suggested here

JFTR, proprosals by @prdoyle

• Expose some of the guts of of getProxy so the user can directly create proxies using the client inside ToxiproxyContainer
• Allocate my own ToxiproxyClient for the ToxiproxyContainer, manage my own port numbers and proxy names. Basically take full control myself and try not to interfere with ToxiproxyContainer's own port numbers or proxy names
• Make my own container, not using ToxiproxyContainer, and use ToxiproxyClient directly on that

and my replied

I have been thinking about option two but since have never used toxiproxy need to tries and solve some doubts taking into account the client.reset(). Please take into account that it shouldn’t be testcontainers toxiproxy module responsibility about providing such helpers around the client as we have it right now. I think that’s the main reason why we can not find a clean solution. TC toxiproxy should be responsible for helping start the container and handle different scenarios around it like in kafka or pulsar for example but shouldn’t be attached to offering a toxiproxy client per se. I think that so far we can think on deprecating some methods, extend documentation and provide a simple way to create toxiproxy clients

[prdoyle]

Ok, I'm trying this and it seems quite cumbersome. ContainerProxy is much handier than plain old Proxy, especially the setConnectionCut method.

I suppose we could see if we can get some of the nice ContainerProxy functionality merged upstream into the Proxy class?

[prdoyle]

I've implemented a workaround for the time being, where I run disruptive tests sequentially so they don't interfere with each other.

I wish I had more time to invest in this. I'm fine with the decision not to support this "subscripts" feature and I'm cool closing this issue if that's how you'd like to proceed.

Thanks very much for the feedback!

[eddumelendez]

@prdoyle glad to hear it! Do you think is worth to document it somewhere? We can discuss it via slack.