refactor: split out association handling in packet handler #221

sbruens · 2024-11-15T21:22:53Z

This allows us to re-use the association handling logic in the Caddy server, where the NAT is handled by Caddy.

The NAT handling for the legacy server is split out into its own PacketServe function (to handle PacketConn types) that is similar to our StreamServe function (to handle StreamConn types).

This will allow us to re-use the handling of packets in the Caddy server where serving is handled separately.

service/udp.go

fortuna

I believe this needs a redesign. We can make the shared abstraction be an association handler, not a packet handler. It significantly simplifies things because the handler has the association context, rather than having the context of an individual packet only.

And it will likely use less memory, since the handler can call read directly to the buffer it uses.

We will need to extract a NAT component for outline-ss-server.
Note that the handler is still the one to dictate the lifetime of an association, by closing the writes. You can't do that with a packet handler.

service/shadowsocks.go

service/udp.go

service/shadowsocks.go

service/udp.go

Instead, make the connection an association by wrapping it with the client address and pass that to the `Handle()` function. Each connection will call `Handle()` only once.

cmd/outline-ss-server/main.go

service/udp.go

sbruens

Split the interface into a ConnAssociation and a PacketAssociation. Is that what you had in mind?

service/udp.go

fortuna

The Association concept is a bit confusing and can be decoupled. Rather than two parallel concepts, make the Caddy handling be based on the PacketAssociation, so you have only one association type.

caddy/shadowsocks_handler.go

service/shadowsocks.go

service/udp.go

…ssociation)`.

service/udp.go

service/shadowsocks.go

service/udp.go

sbruens · 2025-01-08T20:19:37Z

service/udp.go

+	// released after the packet is processed.
+	HandlePacket(pkt []byte, lazySlice slicepool.LazySlice)
+
+	// Read reads data from the association.


I don't feel great about adding a Read() here but otherwise we need HandleAssociation(net.Conn, PacketAssociation) despite PacketAssociation already wrapping the net.Conn (which is needed so that the natmap can close all associations when the natmap is closed).

fortuna

I'm a bit confused with some of the concepts. It may be purely naming. It needs some tweaks to clarify what is going on

service/udp.go

service/shadowsocks.go

cmd/outline-ss-server/main.go

service/shadowsocks.go

fortuna · 2025-01-10T20:01:53Z

service/udp.go

+	// pkt contains the raw packet data.
+	// lazySlice is the LazySlice that holds the pkt buffer, which should be
+	// released after the packet is processed.
+	HandlePacket(pkt []byte, lazySlice slicepool.LazySlice)


This is confusing. The association and the packet handling should be decoupled. We should be able to use different implementations to handle an association.

service/shadowsocks.go

fortuna · 2025-01-10T20:07:23Z

service/shadowsocks.go

 	}
-	s.sh.Handle(ctx, conn, connMetrics)
+	HandleAssociation(assoc)


This sounds like you are using a default handler.
This should probably take an association and a handler.

I think the association is handling itself. That needs to be decoupled.

sbruens force-pushed the sbruens/udp-split-serving branch 5 times, most recently from 56aa501 to 37414f9 Compare November 19, 2024 15:18

refactor: split UDP serving from handling of packets

b2aa859

This will allow us to re-use the handling of packets in the Caddy server where serving is handled separately.

sbruens force-pushed the sbruens/udp-split-serving branch from 37414f9 to b2aa859 Compare November 19, 2024 16:50

sbruens added 2 commits November 20, 2024 13:52

Use a read channel.

c0c0e9f

Rename wrappedPacketConn to just packetConn.

8d95309

sbruens force-pushed the sbruens/udp-split-serving branch from c830973 to 8d95309 Compare November 21, 2024 17:19

Update shadowsocks_handler.

51d5c88

sbruens marked this pull request as ready for review November 22, 2024 18:05

sbruens requested a review from a team as a code owner November 22, 2024 18:05

sbruens requested a review from fortuna November 22, 2024 18:05

sbruens commented Nov 22, 2024

View reviewed changes

service/udp.go Outdated Show resolved Hide resolved

fortuna requested changes Nov 22, 2024

View reviewed changes

service/shadowsocks.go Outdated Show resolved Hide resolved

service/udp.go Outdated Show resolved Hide resolved

service/shadowsocks.go Outdated Show resolved Hide resolved

service/udp.go Outdated Show resolved Hide resolved

sbruens added 10 commits November 22, 2024 15:14

Change the HandlePacket API to not require the pkt.

422542b

Remove the NAT map from the Handle() function.

c10176f

Instead, make the connection an association by wrapping it with the client address and pass that to the `Handle()` function. Each connection will call `Handle()` only once.

Rework the metrics now that the NAT is no longer done by the handler.

004c6c3

Move the metrics AddClosed() call to after the timedCopy returns.

a91d55b

Remove the explicit targetConn.Close() and let it expire on its own.

1d7200b

Add the NAT metrics back in at the server level.

4b8eeae

Use buffer pool on packetHandler instead of global.

1e89e85

Revert wrapping the clientConn with shadowsocks encryption/decryption.

b4e7dbb

Rename packet to association.

63c2cff

Fix tests.

1eeb4d6

sbruens changed the title ~~refactor: split UDP serving from packet handling~~ refactor: introduce a handler that handles a single association Nov 28, 2024

sbruens added 2 commits November 27, 2024 22:10

Update the docstring for PacketServe.

08e8bba

Fix comment to refer to "associations".

34a01d7

sbruens changed the title ~~refactor: introduce a handler that handles a single association~~ refactor: create a handler that handles a single association Nov 28, 2024

Format.

418c0e4

sbruens requested a review from fortuna December 19, 2024 22:02

Do not unpack first packets twice.

98988b0

sbruens changed the title ~~refactor: create a handler that handles a single association~~ refactor: split out association handling in packet handler Dec 20, 2024

Move handling into the association.

b9c0286

sbruens force-pushed the sbruens/udp-split-serving branch from afcdb28 to 07984ed Compare December 20, 2024 20:23

Merge branch 'master' into sbruens/udp-split-serving

d14ea20

sbruens force-pushed the sbruens/udp-split-serving branch from 07984ed to d14ea20 Compare December 20, 2024 20:33

Add some comments to the timeout value.

eef630a

sbruens commented Dec 20, 2024

View reviewed changes

cmd/outline-ss-server/main.go Show resolved Hide resolved

Don't set the stream dialer in the old config flow.

1c462b1

fortuna reviewed Jan 6, 2025

View reviewed changes

service/udp.go Outdated Show resolved Hide resolved

service/udp.go Outdated Show resolved Hide resolved

service/udp.go Outdated Show resolved Hide resolved

sbruens added 7 commits January 6, 2025 13:23

Rename AddAuthentication and AddClose.

0918050

Update comment to reflect it handles packets from both directions.

78af4be

Separate the interfaces for Handle() and HandlePacket().

2cf398c

Fix typo in UDPAssocationMetrics.

1055cb1

Don't pass conn to Handle().

f8ab81a

Update comment.

e6ef2f3

Add comments.

2939f8a

sbruens requested a review from fortuna January 6, 2025 19:21

sbruens commented Jan 6, 2025

View reviewed changes

service/udp.go Outdated Show resolved Hide resolved

service/udp.go Outdated Show resolved Hide resolved

fortuna requested changes Jan 7, 2025

View reviewed changes

caddy/shadowsocks_handler.go Outdated Show resolved Hide resolved

service/shadowsocks.go Outdated Show resolved Hide resolved

service/udp.go Outdated Show resolved Hide resolved

service/udp.go Outdated Show resolved Hide resolved

service/udp.go Outdated Show resolved Hide resolved

sbruens added 4 commits January 8, 2025 12:55

Remove ConnAssociation in favor of a `HandleAssociation(Conn, PacketA…

23a03e9

…ssociation)`.

Split Service interface into outline-ss-server and Caddy interfaces.

aa130f0

Remove unused const.

19fb5f7

Don't require conn in HandleAssociation().

c656b36

sbruens commented Jan 8, 2025

View reviewed changes

Exit the loop if the connection is closed.

31cec7c

sbruens requested a review from fortuna January 9, 2025 20:32

fortuna requested changes Jan 10, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: split out association handling in packet handler #221

refactor: split out association handling in packet handler #221

sbruens commented Nov 15, 2024 •

edited

Loading

fortuna left a comment

sbruens left a comment

fortuna left a comment

sbruens Jan 8, 2025 •

edited

Loading

fortuna left a comment

fortuna Jan 10, 2025

fortuna Jan 10, 2025

refactor: split out association handling in packet handler #221

Are you sure you want to change the base?

refactor: split out association handling in packet handler #221

Conversation

sbruens commented Nov 15, 2024 • edited Loading

fortuna left a comment

Choose a reason for hiding this comment

sbruens left a comment

Choose a reason for hiding this comment

fortuna left a comment

Choose a reason for hiding this comment

sbruens Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

fortuna left a comment

Choose a reason for hiding this comment

fortuna Jan 10, 2025

Choose a reason for hiding this comment

fortuna Jan 10, 2025

Choose a reason for hiding this comment

sbruens commented Nov 15, 2024 •

edited

Loading

sbruens Jan 8, 2025 •

edited

Loading