Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(client): dynamic key config parsing regression #2202

Closed
wants to merge 1 commit into from

Conversation

jyyi1
Copy link
Contributor

@jyyi1 jyyi1 commented Sep 9, 2024

This PR fixes a regression caused by #2192 , where the dynamic key JSON fields server and server_port were ignored.

The Go code now accepts both host+port and server+server_port.

The following screenshots are taken when using this key for test : https://github.com/jyyi1/jyyibin/raw/main/keys/unreachable . We expect a ServerUnreachable error, instead of a UnexpectedPluginError.

Expected (Left) v.s. Current (Right)

@jyyi1 jyyi1 marked this pull request as ready for review September 9, 2024 20:13
@jyyi1 jyyi1 requested a review from a team as a code owner September 9, 2024 20:13
@jyyi1 jyyi1 requested review from fortuna and sbruens September 9, 2024 20:14
Copy link
Contributor

@sbruens sbruens left a comment

Choose a reason for hiding this comment

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

Can you add a regression test for this?

Copy link
Collaborator

@fortuna fortuna left a comment

Choose a reason for hiding this comment

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

This is a launch blocker

@@ -52,6 +52,9 @@ type configJSON struct {
Password string `json:"password"`
Method string `json:"method"`
Prefix string `json:"prefix"`

DynamicKeyServer string `json:"server"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Having many ways to express the same thing in the code is a pain. Please don't.
That's convenient for providers, but makes it hard to write correct code, as evidenced by the bug you are trying to fix. It's also a pain for things like getting the host or address from the config.

We need to normalize the config as soon as we get it, so all our code works with a single single way to represent the config.

I'm in the process of normalizing the config already: #2203

Please revert this change to the Go code and fix in the TS code. BTW, what changed that broke this? I couldn't pinpoint in my PR.

Copy link
Contributor Author

@jyyi1 jyyi1 Sep 9, 2024

Choose a reason for hiding this comment

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

Here is the code that broke the behavior: https://github.com/Jigsaw-Code/outline-apps/pull/2192/files#r1750971442

But should TS code care about the format of the config? I think TS will only treat it as an opaque string, and pass this string to Go. So in the end, Go will be the single source of truth that understands the config format.

We can align with the Shadowsocks format of server+server_port, and get rid of the internal host+port.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Unfortunately TS needs to know about the config for a few reasons:

  • to validate on adding keys
  • to extract the server name for display
  • to resolve the server name on Electron
  • to access tunnel configuration in the application-specific code

We won't be getting rid of those immediately. We may move some of that to Go, but it will require a new Go target. For now, let's stick to TS, and pass the normalized config to Go.

For the issue you are trying to fix, let's fix where we parse the JSON in Typescript.

@fortuna
Copy link
Collaborator

fortuna commented Sep 9, 2024

This is very confusing. De we even support host + port? Is that documented anywhere?

@jyyi1
Copy link
Contributor Author

jyyi1 commented Sep 9, 2024

This is very confusing. De we even support host + port? Is that documented anywhere?

No, we only publicly support server+server_port. The host+port is the internal format.

Copy link
Collaborator

@fortuna fortuna left a comment

Choose a reason for hiding this comment

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

I'll fix the bug and a dedicated PR, don't worry.

@jyyi1
Copy link
Contributor Author

jyyi1 commented Sep 10, 2024

Close this in favor of #2204

@jyyi1 jyyi1 closed this Sep 10, 2024
@jyyi1 jyyi1 deleted the junyi/fix-config-regression branch September 11, 2024 19:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants