quic: add `DisableReuseport` option #1476

gfanton · 2022-05-04T16:17:43Z

Moved from libp2p/go-libp2p-quic-transport#272

Hey, I just started to add the DisableReuseport options to quic transport. It seems to be working as expected with some cellular operators that couldn't maintain the connection (I tried with this script https://github.com/gfanton/libp2p-reuseport-test)

Just some few things:

It seems that there is no option/configuration mechanism, so I added one. Let me know if you are ok with this or if you want to use another method to pass the argument.
I wrapped all the conn_tests with a simple table test case to test everything conn related with the reuseport_on/off
I have to use net.ListenUDP instead of net.DialUDP for the Dial method to work, otherwise the call to the WriteTo method fails.

Also there are two tests that currently fail with the reuseport disabled:

TestHolePunching

=== RUN   TestHolePunching/reuseport_off
    conn_test.go:59: using a Secp256k1 key: 16Uiu2HAmPxEseg6Hti88hpHTyUB3tQk9bw7paj9yS3rPkBgPnWgM
    conn_test.go:59: using a Ed25519 key: 12D3KooWAaU8sc9j783H9hH3eG3yyJDnvz9Le1LdhCQFbYe5gFyu
    conn_test.go:581: 
        	Error Trace:	conn_test.go:581
        	           				asm_arm64.s:1133
        	Error:      	An error is expected but got nil.
        	Test:       	TestHolePunching/reuseport_off
        	Messages:   	didn't expect to accept any connections
    conn_test.go:602: 
        	Error Trace:	conn_test.go:602
        	Error:      	Condition never satisfied
        	Test:       	TestHolePunching/reuseport_off

TestStatelessReset

=== RUN   TestStatelessReset/reuseport_off
    conn_test.go:59: using a ECDSA key: QmRoF9AeMPnxYMrErq91y2xCvjiULp9n9cJuVwDHu9Kdao
    conn_test.go:59: using a RSA key: QmNvwzR7VDFy1wFdHE3PgV8mCztJbFWz1d5NXVL3wXYVgX
    conn_test.go:534: 
        	Error Trace:	conn_test.go:66
        	           				conn_test.go:534
        	Error:      	Received unexpected error:
        	           	listen udp4 127.0.0.1:55213: bind: address already in use
        	Test:       	TestStatelessReset/reuseport_off

Do these tests make sense when used with resuseport disabled?

marten-seemann · 2022-05-26T11:36:54Z

The QUIC tests were very flaky, and we've cleaned them up since. Could you rebase this PR on the current master?

gfanton · 2022-08-11T13:38:29Z

I just rebased the branch on master, it seems that all the concerned tests pass now!

marten-seemann

This mostly looks good. I added a few comments.

marten-seemann · 2022-08-12T16:54:17Z

p2p/transport/quic/conn_test.go

-		require.Equal(t, serverConn.RemotePeer(), clientID)
-		require.True(t, serverConn.RemotePublicKey().Equals(clientKey.GetPublic()), "remote public key doesn't match")
-	}
+	for _, tc := range connTestCases {


Please introduce a separate function, so you don't have to indent everything here.

marten-seemann · 2022-08-12T16:54:29Z

p2p/transport/quic/conn_test.go

 }

 func TestResourceManagerSuccess(t *testing.T) {
-	serverID, serverKey := createPeer(t)
-	clientID, clientKey := createPeer(t)
+	for _, tc := range connTestCases {


marten-seemann · 2022-08-12T16:55:06Z

p2p/transport/quic/options.go

+
+type Option func(opts *Config) error
+
+type Config struct {


This doesn't need to be exported.

marten-seemann · 2022-08-12T16:57:11Z

p2p/transport/quic/transport.go

 	if err != nil {
 		return nil, err
 	}
-	return reuse.Listen(network, laddr)
+	return newNoReuseConn(conn), nil


Nit: This doesn't need a constructor, just use noreuseConn{Conn: conn}.

Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>

close listener underlying connection when reuseport is disabled and the close method called Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>

gfanton · 2022-08-16T14:50:22Z

I've made the requested changes, but I've missed some DisableReuseport option in some test and still have the same issues described above with TestHolePunching & TestStatelessReset:

for TestStatelessReset, I managed to fix it in my last commit by closing the connection with listener.Close releasing the used port.
for TestHolePunching I'm still facing the same issue, but I'm not really sure that this test has any interest with reuseport disabled. Do you have any clue about this ?

thanks !

marten-seemann · 2022-08-16T15:34:45Z

Hole punching is only expected to work with reuseport enabled. You can disable that test for the non-reuseport version (please add a comment why!).

Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>

gfanton · 2022-08-16T16:05:30Z

done 👍

gfanton · 2022-08-19T09:09:14Z

One test is failing, but it doesn't seem to be related to my PR.

gfanton force-pushed the feat/quic-disable-reuseport branch from 1365b5e to 0173e41 Compare May 5, 2022 08:38

marten-seemann self-requested a review May 5, 2022 08:50

gfanton force-pushed the feat/quic-disable-reuseport branch from 0173e41 to e88abfa Compare June 8, 2022 14:10

gfanton force-pushed the feat/quic-disable-reuseport branch from e88abfa to 8c5b669 Compare August 11, 2022 13:08

gfanton marked this pull request as ready for review August 11, 2022 13:08

marten-seemann reviewed Aug 12, 2022

View reviewed changes

marten-seemann added the need/author-input Needs input from the original author label Aug 12, 2022

gfanton force-pushed the feat/quic-disable-reuseport branch from 8327714 to f202713 Compare August 16, 2022 14:11

feat: add DisableReuseport option

21d5011

Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>

gfanton force-pushed the feat/quic-disable-reuseport branch from f202713 to eacccb5 Compare August 16, 2022 14:18

fix: TestStatelessReset/reuseport_off test

93bf6d6

close listener underlying connection when reuseport is disabled and the close method called Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>

gfanton force-pushed the feat/quic-disable-reuseport branch from eacccb5 to 93bf6d6 Compare August 16, 2022 14:47

fix: skip DisableReuseport option on TestHolePunching

cedb62d

Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>

gfanton requested a review from marten-seemann August 19, 2022 09:04

marten-seemann changed the title ~~feat: add DisableReuseport option to quic-transport~~ quic: add DisableReuseport option Aug 19, 2022

marten-seemann mentioned this pull request Aug 19, 2022

go-libp2p v0.23 #1690

Closed

21 tasks

marten-seemann approved these changes Aug 19, 2022

View reviewed changes

marten-seemann removed the need/author-input Needs input from the original author label Aug 19, 2022

marten-seemann merged commit 0f4a969 into libp2p:master Aug 19, 2022

gfanton deleted the feat/quic-disable-reuseport branch August 19, 2022 11:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

quic: add `DisableReuseport` option #1476

quic: add `DisableReuseport` option #1476

gfanton commented May 4, 2022

marten-seemann commented May 26, 2022

gfanton commented Aug 11, 2022 •

edited

Loading

marten-seemann left a comment

marten-seemann Aug 12, 2022

marten-seemann Aug 12, 2022

marten-seemann Aug 12, 2022

marten-seemann Aug 12, 2022

gfanton commented Aug 16, 2022 •

edited

Loading

marten-seemann commented Aug 16, 2022

gfanton commented Aug 16, 2022

gfanton commented Aug 19, 2022

quic: add DisableReuseport option #1476

quic: add DisableReuseport option #1476

Conversation

gfanton commented May 4, 2022

marten-seemann commented May 26, 2022

gfanton commented Aug 11, 2022 • edited Loading

marten-seemann left a comment

Choose a reason for hiding this comment

marten-seemann Aug 12, 2022

Choose a reason for hiding this comment

marten-seemann Aug 12, 2022

Choose a reason for hiding this comment

marten-seemann Aug 12, 2022

Choose a reason for hiding this comment

marten-seemann Aug 12, 2022

Choose a reason for hiding this comment

gfanton commented Aug 16, 2022 • edited Loading

marten-seemann commented Aug 16, 2022

gfanton commented Aug 16, 2022

gfanton commented Aug 19, 2022

quic: add `DisableReuseport` option #1476

quic: add `DisableReuseport` option #1476

gfanton commented Aug 11, 2022 •

edited

Loading

gfanton commented Aug 16, 2022 •

edited

Loading