http_proxy: support multi-addresses listening #654

Choraden · 2024-01-17T15:37:43Z

This patch adds multi-addresses listener.

Fixes #629

bind/flag.go

mmatczuk · 2024-01-18T09:38:28Z

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 · 2024-01-18T09:41:01Z

net.go

@@ -89,24 +91,64 @@ type ListenerCallbacks interface {

 	// OnTLSHandshakeError is called after a TLS handshake errors out.
 	OnTLSHandshakeError(*tls.Conn, error)
+
+	// OnBindError is called when a listener fails to bind to an address.
+	OnBindError(address string, err error)


Should go before TLS things.

mmatczuk · 2024-01-18T09:41:31Z

net.go

 type Listener struct {
-	Address          string
+	Addresses        []string


Listener changes should be in a separate commit.

mmatczuk · 2024-01-18T09:42:24Z

net.go

 	Log              log.Logger
 	TLSConfig        *tls.Config
 	HandshakeTimeout time.Duration
 	ReadLimit        int64
 	WriteLimit       int64
 	Callbacks        ListenerCallbacks

-	listener net.Listener
+	ls        []net.Listener


Why not listeners

mmatczuk · 2024-01-18T09:45:19Z

net.go

+			if l.Callbacks != nil {
+				l.Callbacks.OnBindError(addr, err)
+			}
+			if i == 0 {


I think that when using more than one port they are all equal.

mmatczuk · 2024-01-18T09:48:22Z

net.go

+	return ll, nil
+}
+
+func (l *Listener) acceptLoop(ll net.Listener) {


I'd use errgroup with anonymous function instead of creating this and closeCh.

mmatczuk · 2024-01-18T09:49:05Z

net_test.go

@@ -70,3 +122,6 @@ func (m *mockListenerCallback) OnTLSHandshakeError(_ *tls.Conn, err error) {
 	}
 	m.donec <- struct{}{}
 }
+


mmatczuk · 2024-01-18T09:50:27Z

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

This patch moves tls configuration from http.Server to forwarder.Listener. It makes TLSHandshakeTimeout applicable.

The optional addresses to listen on. Bind to this addresses may fail.

Fixes #629

mmatczuk · 2024-01-29T10:25:05Z

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 · 2024-01-29T10:42:47Z

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 · 2024-01-30T02:19:23Z

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 #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... #629 ?

mmatczuk · 2024-01-30T08:46:44Z

@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 · 2024-01-31T21:40:45Z

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 · 2024-02-02T10:51:14Z

Let's split the work and move the port forwarding to separate feature #669

waggledans reviewed Jan 17, 2024

View reviewed changes

bind/flag.go Outdated Show resolved Hide resolved

mmatczuk reviewed Jan 18, 2024

View reviewed changes

net.go Outdated

type Listener struct {

Address string

Addresses []string

Copy link

Contributor

mmatczuk Jan 18, 2024

Choose a reason for hiding this comment

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

Listener changes should be in a separate commit.

mmatczuk reviewed Jan 18, 2024

View reviewed changes

Choraden force-pushed the hg/multi_listen_v2 branch 7 times, most recently from 966c19a to e8fde91 Compare January 19, 2024 14:41

Choraden marked this pull request as ready for review January 19, 2024 14:51

Choraden requested a review from a team as a code owner January 19, 2024 14:51

Choraden added 8 commits January 22, 2024 10:47

listener: support multi-addresses listening

9faaeec

http_server: use forwarder's custom Listener

d3880cb

This patch moves tls configuration from http.Server to forwarder.Listener. It makes TLSHandshakeTimeout applicable.

http_server: extend config with optional addresses

e1a3c27

The optional addresses to listen on. Bind to this addresses may fail.

http_server: log all addresses the server is listening on

7428e79

http_proxy: support listening on optional addresses

db02647

Fixes #629

http_proxy: log all addresses the proxy is listening on

74f5e2f

bind: add --optional-addresses flag

d9ac6a1

e2e: add optional addresses flag test

4b67218

Choraden force-pushed the hg/multi_listen_v2 branch from e8fde91 to 4b67218 Compare January 22, 2024 09:50

Choraden requested a review from mmatczuk January 22, 2024 10:56

Choraden closed this Feb 7, 2024

Choraden deleted the hg/multi_listen_v2 branch February 7, 2024 07:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http_proxy: support multi-addresses listening #654

http_proxy: support multi-addresses listening #654

Choraden commented Jan 17, 2024 •

edited

Loading

mmatczuk commented Jan 18, 2024 •

edited

Loading

mmatczuk Jan 18, 2024

mmatczuk Jan 18, 2024

mmatczuk Jan 18, 2024

mmatczuk Jan 18, 2024

mmatczuk Jan 18, 2024

mmatczuk Jan 18, 2024

mmatczuk commented Jan 18, 2024

mmatczuk commented Jan 29, 2024

mmatczuk commented Jan 29, 2024 •

edited

Loading

waggledans commented Jan 30, 2024

mmatczuk commented Jan 30, 2024 •

edited

Loading

waggledans commented Jan 31, 2024

mmatczuk commented Feb 2, 2024

http_proxy: support multi-addresses listening #654

http_proxy: support multi-addresses listening #654

Conversation

Choraden commented Jan 17, 2024 • edited Loading

mmatczuk commented Jan 18, 2024 • edited Loading

mmatczuk Jan 18, 2024

Choose a reason for hiding this comment

mmatczuk Jan 18, 2024

Choose a reason for hiding this comment

mmatczuk Jan 18, 2024

Choose a reason for hiding this comment

mmatczuk Jan 18, 2024

Choose a reason for hiding this comment

mmatczuk Jan 18, 2024

Choose a reason for hiding this comment

mmatczuk Jan 18, 2024

Choose a reason for hiding this comment

mmatczuk commented Jan 18, 2024

mmatczuk commented Jan 29, 2024

mmatczuk commented Jan 29, 2024 • edited Loading

waggledans commented Jan 30, 2024

mmatczuk commented Jan 30, 2024 • edited Loading

waggledans commented Jan 31, 2024

mmatczuk commented Feb 2, 2024

Choraden commented Jan 17, 2024 •

edited

Loading

mmatczuk commented Jan 18, 2024 •

edited

Loading

mmatczuk commented Jan 29, 2024 •

edited

Loading

mmatczuk commented Jan 30, 2024 •

edited

Loading