http_proxy: support multi-addresses listening

[Choraden]

This patch adds multi-addresses listener.

Fixes #629

[mmatczuk]

I'd suggest to extend the port syntax we use in the current flag

flag := [<host>]:<port_expr>
port_expr := <port>|<port-range>,...
port-range := <port>-<port>

Example:

--address localhost:50,60,70-80

Maybe the port range is too much and can be ignored.

The logic should be that we shall always be able to listen to one or more ports.

[mmatczuk]

As for the integration, the main part should be in the listener and it should go to the HttpServer.

[mmatczuk]

We have Listen and Listener struct, http server using the former http proxy using the latter. This is ok but you can see that the responsibility is duplicated.

I suggest the following refactoring

• Make Listener based on net.Listener not Address
• Add MultiListener like https://github.com/daniel-garcia/multilistener, can be our own - the idea here is seal the hanging multiple listeners in one place and avoid using it if not needed
• Use MultiListener in http proxy only if absolutely needed.

The MultiListeners idea may not be the best in general

• Connection can be accepted and not read from - blocked on channel put
• Channel is a bottleneck and a back pressure mechanism at the same time It's hard to tell it's definitely bad without any benchmarks, but we should be aware of it and maybe add some benchmarks? The good thing about it is that it allows for easy reuse of our other code in Listener.

Having said that maybe the Listener struct should be removed altogether. The rate limiting is already a net.Listener wrapper, how about we refactored the Listener tls code into a net.Listener wrapper?

I'm thinking we should move net.go to a dedicated package ex. utils/netx, the rate limit could go under it.

[mmatczuk]

The --address and --optional-addresses is a usability mess it immediately raises questions like why would you ever need that.

All of that would not be a problem if we focused on file based configuration only - say we would have listener config object like Envoy. Then you could add optional field to it. We also have protocol and bunch of other stuff that make adding multiple addresses imbalance other configurations.

Maybe we are solving wrong problem and this should be done on a script level?

[waggledans]

Maybe we are solving wrong problem and this should be done on a script level?

What do you have in mind? The problem is described in https://github.com/saucelabs/forwarder/issues/629

The --address and --optional-addresses is a usability mess

I'd rather have some way to assign properties to listeners, not sure how to make it work with CLI.

it immediately raises questions like why would you ever need that.

hmm... https://github.com/saucelabs/forwarder/issues/629 ?

[mmatczuk]

@waggledans #629 points to a doc that says

Microsoft Edge, Chrome 71+, and the Safari browser on OS X 10.10+ and mobile iOS 8+ proxy these common ports

What that even means? Why we should so easily give up on the ports. What's the purpose?

Why not use a script to launch a bunch of forwarders instead - what it the benefit of integrating it.

[waggledans]

What that even means?, Why we should so easily give up on the ports, What's the purpose?

All these questions indicate that we need to improve Sauce Labs docs if it's not clear from the description. Let's have this discussion elsewhere (we should have had it BEFORE the implementation).

Why not use a script to launch a bunch of forwarders instead

Absolutely not. It's 20-30 ports, I don't think so.

[mmatczuk]

Let's split the work and move the port forwarding to separate feature https://github.com/saucelabs/forwarder/issues/669