feat(client): introduce extensible YAML config in Go #2306

fortuna · 2024-12-09T22:30:00Z

This PR moves the config parsing from Typescript to Go. The new implementation supports an extensible YAML format that is backwards-compatible with the two formats we supported (legacy SS JSON and SS URL).

The migration of parsing from Typescript to Go forced the functions to be changed to async, which cascades on a number of changes up the stack all the way to main. Constructors had to be replaced with async factory functions and web components had to have async updates. I also had to hunt unhandled promise rejections at run time, since Typescript is unable to detect that.

TODO:

Replace Typescript tests with Go tests
Cleanup Endpoint code to use ExtensibleProvider now that it supports dispatching based on config type
Define final format

fortuna · 2025-01-06T18:49:26Z

I still need to fix the Electron test, but I think we can start the review process.

sbruens

Overall looks great. I think the config package could use some more liberal commenting on exported types (e.g. in module.go), but I'll leave it up to you which ones to focus the reader's attention on.

sbruens · 2025-01-06T19:30:49Z

client/go/outline/config/config.go

+	}
+	yamlText, err := yaml.Marshal(newMap)
+	if err != nil {
+		return err


It may be beneficial to format this error since you can also run into errors with decoding below. That will make it easier to debug later:

if err != nil { return fmt.Errorf("error marshaling to YAML: %w", err) } ... if err := decoder.Decode(out); err != nil { return fmt.Errorf("error decoding YAML: %w", err) } return nil

sbruens · 2025-01-06T19:35:19Z

client/go/outline/config/config.go

+		}
+		newMap[k] = v
+	}
+	yamlText, err := yaml.Marshal(newMap)


What's the overhead of marshaling and then immediately unmarshalling below? That seems somewhat inefficient.

What's the purpose of this marshal/unmarshal step? Is it just to filter out fields? I think you may be able to use reflection to create the out struct directly?

This is going from a map to a struct. Yes, I could use reflect, but it's not that straightforward. I decided to leverage the YAML code to save time.

I'm not worried about performance here, since it's a relatively small object and this is only done at config parsing time.

sbruens · 2025-01-06T19:40:31Z

client/go/outline/config/config_test.go

+	"github.com/stretchr/testify/require"
+)
+
+/*


Delete these commented out tests?

sbruens · 2025-01-06T20:02:40Z

client/go/outline/config/module_test.go

+	"github.com/stretchr/testify/require"
+)
+
+// TODO:


Are these tests not already working? They are the main way to see how this module can be used, so it will be helpful for these tests to be live and comprehensive so they can be self-documenting.

Or are these what you meant by "need to fix electron tests"?

They are working now. Readded

daniellacosse

Looks good! Just a few thoughts/observations

daniellacosse · 2025-01-06T19:05:38Z

client/go/outline/client_test.go

+)
+
+func Test_NewTransport_SS_URL(t *testing.T) {
+	config := "ss://[email protected]:4321/"


might want to test multiple new-line separated ss keys, too

We don't support multiple keys yet.

daniellacosse · 2025-01-06T19:08:23Z

client/go/outline/config/config.go

+
+const ConfigParserKey = "$parser"
+
+type ConfigNode any


is it not possible to define an interface for this? the parser library doesn't provide one?

Not really. It can be anything.

daniellacosse · 2025-01-06T19:10:02Z

client/go/outline/config/config.go

+	return decoder.Decode(out)
+}
+
+// TypeParser creates objects of the given type T from an input config.


are these objects ConfigNodes or not necessarily?

In this case they are always map, since we can only specify a subparser in a map.

daniellacosse · 2025-01-06T20:10:11Z

client/go/outline/config/endpoint.go

+		},
+		ConnectionProviderInfo: dialer.ConnectionProviderInfo,
+	}
+	if dialer.ConnType == ConnTypeDirect {


Sightly confused, is this a different type of Direct? if not, should this method fail to parse if it isn't?

If the connection is direct from the dialer, then we use the address we are dialing.
If the connection is tunneled, then we use the firstHop from the Dialer.

daniellacosse · 2025-01-06T20:22:42Z

client/src/www/app/outline_server_repository/index.ts

-      return;
-    }
-    this.loadServersV0();
+  async internalCreateServer(


why remove private? because it's only at compile time?

It's needed in loadServersV* and OutlineServerRepository is not exported.

also internal doesn't add any information to the function, maybe createAndRegisterServer

It conveys the idea that this is not supposed to be used as an external API. It's for internal use only.

daniellacosse · 2025-01-06T20:24:04Z

client/src/www/app/outline_server_repository/server.ts

-  readonly tunnelConfigLocation: URL;
-  private displayAddress: string;
-  private readonly staticTunnelConfig?: TunnelConfigJson;
+export async function newOutlineServer(


nit: I prefer make to new to more explicitly call out that you're using a factory function and not a constructor, but I'm ambivalent

New is more aligned with the factory methods in Go. I believe this will be more consistent. Also, we use new in TS to create objects.

jyyi1

Starting with the TypeScript review first. The PR is pretty big, is it feasible to separate it platform-by-platform?

infrastructure/net.ts

jyyi1 · 2025-01-06T23:05:43Z

client/src/www/app/main.cordova.ts

+    try {
+      return pluginExecWithErrorCode('invokeMethod', methodName, params);
+    } catch (e) {
+      console.debug('invokeMethod failed', methodName, e);


error instead of debug? Also, are you planning to catch the error of "initiating" the promise or "during" the promise? This will decide whether we need to add await here.

Suggested change

console.debug('invokeMethod failed', methodName, e);

console.error('invokeMethod failed', methodName, e);

I don't think this should be errors, because the calls may fail for various reasons (like failed to parse the config in the key validation). This is really for debugging. The caller should decide whether to log.

jyyi1 · 2025-01-06T23:20:08Z

go.mod

@@ -1,6 +1,6 @@
 module github.com/Jigsaw-Code/outline-apps

-go 1.21
+go 1.22


Are there any go 1.22 features we are using in this PR? If not, maybe we can separate the PR since it's already pretty big.

I thought there was, but I guess not. Reverted.

jyyi1 · 2025-01-06T23:23:00Z

client/src/cordova/plugin/apple/src/OutlinePlugin.swift

@@ -154,10 +154,12 @@ class OutlinePlugin: CDVPlugin {
    DDLogInfo("Invoking Method \(methodName) with input \(input)")
    Task {
      guard let result = OutlineInvokeMethod(methodName, input) else {
+        DDLogInfo("InvokeMethod \(methodName) got nil result")


Error instead of Info?

Suggested change

DDLogInfo("InvokeMethod \(methodName) got nil result")

DDLogError("InvokeMethod \(methodName) got nil result")

jyyi1 · 2025-01-06T23:23:49Z

client/src/cordova/plugin/apple/src/OutlinePlugin.swift

        return self.sendError("unexpected invoke error", callbackId: command.callbackId)
      }
      if result.error != nil {
        let errorJson = marshalErrorJson(error: OutlineError.platformError(result.error!))
+        DDLogInfo("InvokeMethod \(methodName) failed with error \(errorJson)")


Warning ? Considering it might be user errors.

Suggested change

DDLogInfo("InvokeMethod \(methodName) failed with error \(errorJson)")

DDLogWarn("InvokeMethod \(methodName) failed with error \(errorJson)")

I disagree with this one. The invoked method may fail for various reasons (e.g. connectivity) and it's not necessarily something to report in the logs here. It's up to the caller to decide.
I had this to debug the calls during development.

jyyi1 · 2025-01-06T23:31:06Z

client/src/www/views/root_view/add_access_key_dialog/index.ts

@@ -105,13 +112,22 @@ export class AddAccessKeyDialog extends LitElement {
        </md-text-button>
        <md-filled-button
          @click=${this.handleConfirm}
-          ?disabled=${!this.accessKey || !this.isValidAccessKey(this.accessKey)}
+          ?disabled=${!this.accessKey || !this.isValidAccessKey}


The validation is async, will the user be able to click this button even if the key is invalid during validation? Take an example of the following sequence:

Key is valid, <Button Enabled>, but user wants to change the key Paste in the new (invalid) key Validating ... <Button still Enabled> <= is the user able to click the button here? Validation done, invalid key! <Button Disabled>

They will, but the add will fail.

jyyi1 · 2025-01-06T23:35:35Z

client/src/www/app/app.ts

@@ -478,7 +480,7 @@ export class App {
      }
    }
    try {
-      config.validateAccessKey(accessKey);
+      config.parseAccessKey(accessKey);


Do we need to await here?

Yes, updated

jyyi1 · 2025-01-06T23:51:02Z

client/src/www/app/outline_server_repository/index.ts

+    storage,
+    localize
+  );
+  await loadServers(storage, repo);


This is the only async code in the constructor, maybe we can add a static function newAndLoadServers() { ... } in OutlineServerRepository, and then marks the OutlineServerRepository.constructor to private. We can still leave other synchronized code in the constructor. I feel this would better encapsulate the creation logic compared to the existing implementation (a caller can call newOutlineServerRepository or new OutlineServerRepository() because it is not private).

export class OutlineServerRepository implements ServerRepository { private constructor(...) { // All existing code except for loadServers } static async createAndLoadServers(...) { const repo = new OutlineServerRepository(...); await loadServers(storage, repo); return repo; } }

I think there's a misunderstanding. OutlineServerRepository is no longer exported. So you can't call the constructor outside this file. newOutlineServerRepository is the only way to create the repo.

jyyi1 · 2025-01-06T23:54:15Z

client/src/www/app/outline_server_repository/server.ts

+  accessKey: string,
+  localize: Localizer
+): Promise<Server> {
+  const serviceConfig = await parseAccessKey(accessKey);


Similar to the comment in OutlineServerRepository, I'd prefer a private constructor of the OutlineServer class and a static async parseKeyAndCreate() in the class.

Same comment. The class is no longer exported, so you must use the factory function.

jyyi1

First round of the Go code review

jyyi1 · 2025-01-07T00:17:34Z

client/src/www/app/outline_server_repository/config.ts

@@ -12,11 +12,12 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.

-import {SHADOWSOCKS_URI} from 'ShadowsocksConfig';
+import * as method_channel from '@outline/client/src/www/app/method_channel';


https://google.github.io/styleguide/tsguide.html#identifiers-imports

Suggested change

import * as method_channel from '@outline/client/src/www/app/method_channel';

import * as methodChannel from '@outline/client/src/www/app/method_channel';

jyyi1 · 2025-01-07T00:26:34Z

client/go/outline/config/types.go

+	ConnTypeTunneled
+)
+
+// ConnProviderConfig represents a dialer or endpoint that can create connections.


Is this comment still valid?

jyyi1 · 2025-01-07T01:26:55Z

client/go/outline/config/parse.go

+// RegisterSubParser registers the given subparser function with the given name for the type T.
+// Note that a subparser always take a map[string]any, not ConfigNode, since we must have a map[string]any in
+// order to set the value for the ConfigParserKey.
+func (p *TypeParser[T]) RegisterSubParser(name string, function func(context.Context, map[string]any) (T, error)) {


This function type is used in several places, shall we define a dedicated type for it?

jyyi1 · 2025-01-07T01:36:54Z

client/src/www/app/outline_server_repository/config.ts

@@ -109,82 +77,44 @@ export function setTransportConfigHost(
 * This is used by the server to parse the config fetched from the dynamic key, and to parse
 * static keys as tunnel configs (which may be present in the dynamic config).
 */
-export function parseTunnelConfig(
+export async function parseTunnelConfig(


The user-provided dynamic key error is removed here, are we still supporting this feature? You can use the following key to test, expect to see "Usage quota exceeded 😭😭😭":

https://raw.githubusercontent.com/jyyi1/jyyibin/refs/heads/main/keys/error_msg.okey#Dynamic%20Error%20Msg

jyyi1 · 2025-01-07T01:43:22Z

client/go/outline/config/module.go

+)
+
+// TransportPair provides a StreamDialer and PacketListener, to use as the transport in a Tun2Socks VPN.
+type TransportPair struct {


Do we really need this type? Why not just returning (*Dialer[transport.StreamConn], *PacketListener, error) in all functions?

jyyi1 · 2025-01-07T01:46:52Z

client/go/outline/client.go

-	proxyAddress := net.JoinHostPort(host, fmt.Sprint(port))
-
-	cryptoKey, err := shadowsocks.NewEncryptionKey(cipherName, password)
+func newClientWithBaseDialers(transportConfig string, tcpDialer transport.StreamDialer, udpDialer transport.PacketDialer) (*Client, error) {


We are changing the UDP handler from a PacketListener to a PacketDialer here, will this cause any unexpected errors?

jyyi1

This change is a huge leap forward! Can we split the PR into at least two to make it smaller and easier to review?

The async validate access keys, this can be extracted first without adding the YAML support
Adding YAML to the format (may be further splited if possible)

The YAML parsing logic feels complicated, especially the definitions of **Endpoint **Dialer, Listener, not sure whether we can simplify it. Probably providing the users with a documentation would be useful (this could be done once we migrate the code to SDK).

jyyi1 · 2025-01-07T01:53:47Z

client/go/outline/config/config_shadowsocks_test.go

+			"cipher":   "chacha20-ietf-poly1305",
+			"secret":   "SECRET",
+			"prefix":   "outline-123",
+			"extra":    "NOT SUPPORTED",


Do we need to fail on extra fields? We can simply ignore them.

jyyi1 · 2025-01-07T20:51:31Z

client/go/outline/config/module.go

+
+	streamEndpoints = NewTypeParser(func(ctx context.Context, input ConfigNode) (*Endpoint[transport.StreamConn], error) {
+		// TODO: perhaps only support string here to force the struct to have an explicit parser.
+		return parseDirectDialerEndpoint(ctx, input, streamDialers.Parse)


So this is the fallback dialer if the user doesn't specify the parser type?

jyyi1 · 2025-01-07T20:55:38Z

client/go/outline/config/module_test.go

+	provider := newTestProvider()
+
+	node, err := ParseConfigYAML(`
+$type: ss


$parser or $type?

jyyi1 · 2025-01-07T20:57:49Z

client/go/outline/config/parse.go

+
+// TypeParser creates objects of the given type T from an input config.
+// You can register type-specific sub-parsers that get called when marked in the config.
+// The default value is not valid. Use [NewTypeParser] instead.


If the default value is not valid, the recommended way in Go is to expose an interface instead.

jyyi1 · 2025-01-07T21:09:53Z

client/go/outline/config/parse.go

+	return node, nil
+}
+
+// mapToAny marshalls a map into a struct. It's a helper for parsers that want to


I feel decodeMapToStruct would be a better name?

jyyi1 · 2025-01-07T21:16:15Z

client/go/outline/client.go

-
-	cryptoKey, err := shadowsocks.NewEncryptionKey(cipherName, password)
+func newClientWithBaseDialers(transportConfig string, tcpDialer transport.StreamDialer, udpDialer transport.PacketDialer) (*Client, error) {
+	transportYAML, err := config.ParseConfigYAML(transportConfig)


Are we still accepting the old JSON format here? Existing dynamic keys are still using JSON.

fortuna added 10 commits November 22, 2024 17:58

Initial Implementation

c615901

Add DialEndpoint

a4c894d

Wire new config

0d1b9ec

Add URL

e81cb45

Wire Endpoint

455a2ad

Cleanup

5a85e73

Tweaks

87eaa89

Merge branch 'master' into fortuna-ws-config

fbddc57

Progress

131e564

Make it work

a26708c

github-actions bot added the size/XXL label Dec 9, 2024

fortuna changed the title ~~chore(client): introduce extensible config~~ chore(client): introduce extensible YAML config Dec 9, 2024

fortuna changed the title ~~chore(client): introduce extensible YAML config~~ chore(client): introduce extensible YAML config in Go Dec 9, 2024

fortuna added 17 commits December 9, 2024 19:14

Simplify

1f89c98

More simplification

0aa3dbc

Merge branch 'master' into fortuna-ws-config

e3738e7

Revert Go version

bc20623

Progress

cde35c5

Fixes

0128549

Fix tests

e05542f

Fix

6c199bf

Add test

dbaf0a2

Add default provider

cffd70a

Clean up

d46455e

Update tests

f63d2b9

Revamp config

8df547f

Clean up and TODOs

52dcd84

Naming

5b80741

Clean up test

abcbb42

More tests

70a4a28

fortuna added 4 commits January 2, 2025 10:18

Try Coder

fa05c42

Comment

79c0a73

Remove Websocket

24f0544

Merge branch 'master' into fortuna-ws-config

777da9b

fortuna changed the title ~~chore(client): introduce extensible YAML config in Go~~ feat(client): introduce extensible YAML config in Go Jan 6, 2025

Fix Android

612d5db

fortuna requested review from sbruens and jyyi1 January 6, 2025 18:48

fortuna marked this pull request as ready for review January 6, 2025 18:49

fortuna requested review from a team as code owners January 6, 2025 18:49

sbruens reviewed Jan 6, 2025

View reviewed changes

daniellacosse reviewed Jan 6, 2025

View reviewed changes

fortuna added 6 commits January 6, 2025 16:25

Add net

ebb993f

Fix Electron

01d95d2

Review changes

486aabb

Merge branch 'master' into fortuna-ws-config

fdd54d5

fixes

5ab7ea9

Fix Linux

d18deae

jyyi1 reviewed Jan 6, 2025

View reviewed changes

fortuna added 3 commits January 6, 2025 19:49

Fixes

b4a6e1f

Tests

025f102

Lint

3334519

fortuna requested review from daniellacosse, sbruens and jyyi1 January 7, 2025 01:24

jyyi1 reviewed Jan 7, 2025

View reviewed changes

fortuna mentioned this pull request Jan 7, 2025

Add support for Websocket endpoints #2272

Open

	console.debug('invokeMethod failed', methodName, e);
	console.error('invokeMethod failed', methodName, e);

	DDLogInfo("InvokeMethod \(methodName) got nil result")
	DDLogError("InvokeMethod \(methodName) got nil result")

	DDLogInfo("InvokeMethod \(methodName) failed with error \(errorJson)")
	DDLogWarn("InvokeMethod \(methodName) failed with error \(errorJson)")

	import * as method_channel from '@outline/client/src/www/app/method_channel';
	import * as methodChannel from '@outline/client/src/www/app/method_channel';

feat(client): introduce extensible YAML config in Go #2306

Are you sure you want to change the base?

feat(client): introduce extensible YAML config in Go #2306

Conversation

fortuna commented Dec 9, 2024 • edited Loading

fortuna commented Jan 6, 2025

sbruens 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

daniellacosse 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

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

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

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

fortuna commented Dec 9, 2024 •

edited

Loading