feat(x): make config support PacketListeners and make dependencies explicit and decoupled #304

fortuna · 2024-10-28T22:25:22Z

Properly Support PacketListeners in the Config

As I design the advanced config for Outline, I ran into the issue that we need to support PacketListeners for both the Outline Client and to use QUIC. This PR allows PacketListeners to have a nested config like we have for dialers. I also added support to register types.

Explicit and decouples dependencies

The factory functions used to take as input factory functions for stream and packet dialers, regardless whether they needed them or not. This made dependencies unclear and not enforceable at compile time.

It also doesn't scale well, as more advanced strategies may need dependencies of different types. With the introduction of PacketListeners, I already ran into that issue. However, instead of adding a PacketListener factory to the factory function, I simplified the registration function to only take the context and config. The extra dependencies are passed explicitly then you create the factory function. This ensures compile checks, and allows us to inject any dependency we want, without needing to change the API.

Include Context

I now pass a Context to factory functions. This will allow us to inject deadlines or cancel creation for dialers that take long (e.g. Smart Dialer, Psiphon). It may also allow us to inject a structure for "named dialers" in the future.

fortuna · 2024-10-28T22:26:40Z

BTW, I believe it's possible to reduce some of the duplication with Generics. I'll explore that in parallel, but I wanted to advance this PR regardless of that.

fortuna · 2024-10-28T22:29:33Z

BTW, I am not changing the config format yet, but I introduced a Config object. That can encapsulate the intermediate format for the future config.

The flow is:
string -> intermediate/generic Config -> actual type config.

fortuna · 2024-10-29T04:01:20Z

FYI, example QUIC support using this change: #305

fortuna · 2024-10-29T20:11:25Z

Changed the code to use Generics and save coding.

fortuna · 2024-10-29T20:22:32Z

x/configurl/module.go

Note how I'm separating this module file.

I envision the config.go living in the root SDK, since it's generic and simple, with minimal dependencies, and this config module being in the x folder, since it pulls all sorts of dependencies to implement all protocols.

Once the config lives in the main module, we can also consider adding config packages to each transport in the future too, to make it easy to register them.

jyyi1

Thanks for the refactoring, it is great!

jyyi1 · 2024-10-29T21:23:24Z

x/configurl/config.go

+	_ TypeRegistry[any] = (*ExtensibleProvider[any])(nil)
+)
+
+func (p *ExtensibleProvider[ObjectType]) buildersMap() map[string]BuildFunc[ObjectType] {


Maybe call it ensureBuildersMap since it has side effect to the object?

Suggested change

func (p *ExtensibleProvider[ObjectType]) buildersMap() map[string]BuildFunc[ObjectType] {

func (p *ExtensibleProvider[ObjectType]) ensureBuildersMap() map[string]BuildFunc[ObjectType] {

jyyi1 · 2024-10-29T21:23:57Z

x/configurl/config.go


-	return p
+// ExtensibleProvider creates instances of ObjectType in a way that can be extended via its [TypeRegistry] interface.
+type ExtensibleProvider[ObjectType comparable] struct {


Do we really need the comparable constraint here?

Yes, because we compare it against the zero instance in NewInstance.

jyyi1 · 2024-10-29T21:26:35Z

x/configurl/config.go

+func (p *ExtensibleProvider[ObjectType]) RegisterType(subtype string, newInstance BuildFunc[ObjectType]) error {
+	builders := p.buildersMap()
+	if _, found := builders[subtype]; found {
+		return fmt.Errorf("type %v registered twice", subtype)


Maybe we can replace the old entry? This might be useful in some "override" scenarios.

Agreed. Changed accordingly.

jyyi1 · 2024-10-29T21:28:22Z

x/configurl/config.go

-	p.RegisterStreamDialerType("ws", wrapStreamDialerWithWebSocket)
-	p.RegisterPacketDialerType("ws", wrapPacketDialerWithWebSocket)
+// TypeRegistry registers config types.
+type TypeRegistry[ObjectType any] interface {


T would be good enough, since ObjectType doesn't add anything more meaningful than T?

I find ObjectType clearer than simply T, and it reads better in the comment as well.

jyyi1 · 2024-10-29T21:33:36Z

x/configurl/config.go

 	}

-	if _, found := p.pdBuilders[subtype]; found {
-		return fmt.Errorf("config parser %v for StreamDialer added twice", subtype)
+	newDialer, ok := p.buildersMap()[config.URL.Scheme]


To be consistent with the name in RegisterType.

Suggested change

newDialer, ok := p.buildersMap()[config.URL.Scheme]

newInstance, ok := p.buildersMap()[config.URL.Scheme]

Oops, leftover from the refactoring. Renamed.

jyyi1 · 2024-10-29T22:04:39Z

x/configurl/tls_test.go

 func TestTLS_SNI(t *testing.T) {
-	tlsURL, err := url.Parse("tls:sni=www.google.com")
+	tlsURL, err := ParseConfig("tls:sni=www.google.com")


Suggested change

tlsURL, err := ParseConfig("tls:sni=www.google.com")

config, err := ParseConfig("tls:sni=www.google.com")

jyyi1 · 2024-10-29T22:07:40Z

x/configurl/tlsfrag.go

@@ -0,0 +1,39 @@
+// Copyright 2023 The Outline Authors


Suggested change

// Copyright 2023 The Outline Authors

// Copyright 2024 The Outline Authors

jyyi1 · 2024-10-29T22:29:16Z

x/configurl/dns.go

+			return nil, err
+		}
+		if closer, ok := sd.(io.Closer); ok {
+			defer closer.Close()


Why are we closing the sd here? It will be passed to dns.NewStreamDialer below, will it be invalid after closing?

I think we may just need to close it when we return a non-nil error.

Removed.

I was playing with the concept of cleaning up. We will need to add some mechanism for cleanup, or else we might leak resources. But I'll leave that to a future PR.

jyyi1 · 2024-10-29T22:31:01Z

x/configurl/dns.go

+			return nil, err
+		}
+		if closer, ok := pd.(io.Closer); ok {
+			defer closer.Close()


Same as above.

jyyi1 · 2024-10-29T22:48:47Z

x/configurl/shadowsocks.go

-	// TODO: accept an inner dialer from the caller and pass it to UDPEndpoint
-	ep := &transport.UDPEndpoint{Address: config.serverAddress}
-	return shadowsocks.NewPacketListener(ep, config.cryptoKey)
+func registerShadowsocksPacketListener(r TypeRegistry[transport.PacketListener], typeID string, newPD BuildFunc[transport.PacketDialer]) {


Not for this PR, but I think we can replace all PacketListener with the PacketDialer for Shadowsocks in the future. Are we doing anything special in the PacketListener?

Maaaaybe. That's something to be investigated, and it may increase the server capacity. But it may also break NAT traversal, since it will tie the UDP socket to a single destination, preventing the local address from receiving packets from other addresses not yet contacted. This can in turn break some communication tools.

fortuna

Another look?

fortuna · 2024-10-30T19:47:39Z

x/configurl/config.go

-	p.RegisterStreamDialerType("ws", wrapStreamDialerWithWebSocket)
-	p.RegisterPacketDialerType("ws", wrapPacketDialerWithWebSocket)
+// TypeRegistry registers config types.
+type TypeRegistry[ObjectType any] interface {


I find ObjectType clearer than simply T, and it reads better in the comment as well.

fortuna · 2024-10-30T19:48:06Z

x/configurl/config.go


-	return p
+// ExtensibleProvider creates instances of ObjectType in a way that can be extended via its [TypeRegistry] interface.
+type ExtensibleProvider[ObjectType comparable] struct {


Yes, because we compare it against the zero instance in NewInstance.

fortuna · 2024-10-30T19:50:24Z

x/configurl/config.go

+func (p *ExtensibleProvider[ObjectType]) RegisterType(subtype string, newInstance BuildFunc[ObjectType]) error {
+	builders := p.buildersMap()
+	if _, found := builders[subtype]; found {
+		return fmt.Errorf("type %v registered twice", subtype)


Agreed. Changed accordingly.

fortuna · 2024-10-30T19:51:36Z

x/configurl/config.go

 	}

-	if _, found := p.pdBuilders[subtype]; found {
-		return fmt.Errorf("config parser %v for StreamDialer added twice", subtype)
+	newDialer, ok := p.buildersMap()[config.URL.Scheme]


Oops, leftover from the refactoring. Renamed.

fortuna · 2024-10-30T19:53:17Z

x/configurl/doc.go

-Configurable transports simplifies the way you create and manage transports.
-With the config package, you can use [NewPacketDialer] and [NewStreamDialer] to create dialers using a simple text string.
+Configurable strategies simplifies the way you create and manage strategies.
+With the configurl package, you can use [NewPacketDialer], [NewStreamDialer] and [NewPacketListener] to create objects using a simple text string.


Updated it to refer to the ConfigModule functions.

fortuna · 2024-10-30T20:03:06Z

x/configurl/tlsfrag.go

@@ -0,0 +1,39 @@
+// Copyright 2023 The Outline Authors


fortuna · 2024-10-30T21:44:26Z

x/configurl/module.go

+
+// ConfigModule enables the creation of network objects based on a config. The config is
+// extensible by registering providers for config subtypes.
+type ConfigModule struct {


It's not really "Transports". It could be anything. I may need to include dns.Resolvers there, Stream/Packet Endpoints, or http.Clients. So it's really generic.

We can't use both Factory and Provider. They effectively mean the same thing. We could call it something Provider, but it's not clear what "something" should be, and Provider collides with the basic Provider type we already have.

I'm renaming it to ProviderContainer. What do you think?

I also created some auxiliary functions to make it more flexible.

fortuna · 2024-10-30T22:36:10Z

x/configurl/config.go

+	_ TypeRegistry[any] = (*ExtensibleProvider[any])(nil)
+)
+
+func (p *ExtensibleProvider[ObjectType]) buildersMap() map[string]BuildFunc[ObjectType] {


fortuna · 2024-10-30T22:37:56Z

x/configurl/dns.go

+			return nil, err
+		}
+		if closer, ok := sd.(io.Closer); ok {
+			defer closer.Close()


Removed.

I was playing with the concept of cleaning up. We will need to add some mechanism for cleanup, or else we might leak resources. But I'll leave that to a future PR.

fortuna · 2024-10-30T22:38:00Z

x/configurl/dns.go

+			return nil, err
+		}
+		if closer, ok := pd.(io.Closer); ok {
+			defer closer.Close()


fortuna added 11 commits October 25, 2024 18:15

More extensible config

b15bb9b

Add shadowsocks

845dfa6

Add TLS

470bdd3

Fixes

bb0a827

Add Websocket

550384b

Cleanup

5826709

Refactor split

22a0526

Update Shadowsocks

f2938cf

Remove ConfigToObject for now

17182e0

Introduce registry type

77906f1

Finish

ffc0654

fortuna requested a review from jyyi1 October 28, 2024 22:25

Fix

d3712d1

fortuna added 3 commits October 29, 2024 00:17

Fix test

67a190c

Fix name

6803dde

Use Generics

92eca9d

Add comments

f194150

fortuna commented Oct 29, 2024

View reviewed changes

fortuna added 2 commits October 29, 2024 16:47

Add type assertion

76bcf64

Remove unused Provider

866f9c1

fortuna force-pushed the fortuna-config branch from ba74e88 to 866f9c1 Compare October 29, 2024 20:49

jyyi1 reviewed Oct 29, 2024

View reviewed changes

fortuna added 4 commits October 30, 2024 15:50

Allow registration override

eb793dc

Rename newDialer

a848931

Update doc

0e483c7

Remove register methods

b3fbf2b

fortuna added 6 commits October 30, 2024 16:01

Add comment to SanitizeConfig

308ff7d

Rename config

ffbdf49

Update Copyright line

fadf314

Use Providers

47c9ad6

Cleanup

a716c1f

Remove Close

b3fec8a

fortuna commented Oct 30, 2024

View reviewed changes

fortuna requested a review from jyyi1 October 30, 2024 22:38

Update Go mod

f9b97cd

	func (p *ExtensibleProvider[ObjectType]) buildersMap() map[string]BuildFunc[ObjectType] {
	func (p *ExtensibleProvider[ObjectType]) ensureBuildersMap() map[string]BuildFunc[ObjectType] {

	newDialer, ok := p.buildersMap()[config.URL.Scheme]
	newInstance, ok := p.buildersMap()[config.URL.Scheme]

	tlsURL, err := ParseConfig("tls:sni=www.google.com")
	config, err := ParseConfig("tls:sni=www.google.com")

	// Copyright 2023 The Outline Authors
	// Copyright 2024 The Outline Authors

feat(x): make config support PacketListeners and make dependencies explicit and decoupled #304

Are you sure you want to change the base?

feat(x): make config support PacketListeners and make dependencies explicit and decoupled #304

Conversation

fortuna commented Oct 28, 2024

Properly Support PacketListeners in the Config

Explicit and decouples dependencies

Include Context

fortuna commented Oct 28, 2024

fortuna commented Oct 28, 2024

fortuna commented Oct 29, 2024

fortuna commented Oct 29, 2024

Choose a reason for hiding this comment

jyyi1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fortuna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment