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

feat: introduce address override #155

Merged
merged 8 commits into from
Jan 9, 2024
Merged

feat: introduce address override #155

merged 8 commits into from
Jan 9, 2024

Conversation

fortuna
Copy link
Contributor

@fortuna fortuna commented Jan 2, 2024

This is a feature I've been wanting for a while that helps with debugging and can be used to bypass DNS censorship.

@fortuna fortuna requested a review from jyyi1 January 2, 2024 20:14
Copy link
Contributor

@jyyi1 jyyi1 left a comment

Choose a reason for hiding this comment

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

Code looks good, but the naming is a little bit confusing because override can mean a lot of things. Maybe domain-name-override, or resolve-host, or replace-target-domain would sound better?

x/config/override.go Outdated Show resolved Hide resolved
return func(address string) (string, error) {
host, port, err := net.SplitHostPort(address)
if err != nil {
return "", fmt.Errorf("address is not valid host:port: %w", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of returning an error, we may also return hostOverride + ":" + portOverride here if hostOverride != "" && portOverride != "".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@fortuna
Copy link
Contributor Author

fortuna commented Jan 8, 2024

Comments addressed.

On the naming, it means "address override". We can override the host or the port, so names with "domain" or "host" are not the right scope. "Resolve" also doesn't apply, since we can override the host with a domain.

I thought about using "map", but there's no key to lookup. Perhaps set. It would look like one of these:

  1. override:host=cloudflare.net
  2. set:host=cloudflare.net

override is more specific, since it carries the semantics that an address already exists.

@fortuna fortuna requested a review from jyyi1 January 8, 2024 15:36
return func(address string) (string, error) {
host, port, err := net.SplitHostPort(address)
if err != nil {
return "", fmt.Errorf("address is not valid host:port: %w", err)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -143,7 +145,7 @@ The [`fetch` tool](https://pkg.go.dev/github.com/Jigsaw-Code/outline-sdk/x/examp
a URL, similar to `curl`. The example below would bypass blocking of `meduza.io` in Russia:

```console
$ go run github.com/Jigsaw-Code/outline-sdk/x/examples/fetch@latest -transport "tlsfrag:1" -method HEAD -v -address "cloudflare.net" https://meduza.io/
$ go run github.com/Jigsaw-Code/outline-sdk/x/examples/fetch@latest -transport "override:host=cloudflare.net|tlsfrag:1" -method HEAD -v https://meduza.io/
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the examples to show that we don't need an application change anymore with override.

@jyyi1
Copy link
Contributor

jyyi1 commented Jan 8, 2024

Comments addressed.

On the naming, it means "address override". We can override the host or the port, so names with "domain" or "host" are not the right scope. "Resolve" also doesn't apply, since we can override the host with a domain.

I thought about using "map", but there's no key to lookup. Perhaps set. It would look like one of these:

  1. override:host=cloudflare.net
  2. set:host=cloudflare.net

override is more specific, since it carries the semantics that an address already exists.

What I'm worried about the naming override is what if we would have more stuffs to override in the future (for example, cipher override), shall we put more parameters to override (i.e. override:cipher=xxx&host=yyy), or shall we name it to be a more specific override (i.e. cipher-override, address-override)?

@fortuna
Copy link
Contributor Author

fortuna commented Jan 8, 2024

This is a Dialer specification. The only thing a Dialer sees is an address, so there won't be a need to specify other overrides, unless we introduce some sort of modifier, but that's a whole can of worms.

We could use address-override, but address is redundant given the host and port that is explicitly specified (override:host=$HOST). It would be needed in the absence of the attribute (override-host:$HOST)

Copy link
Contributor

@jyyi1 jyyi1 left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! Let's use override for now.

@fortuna fortuna merged commit 9be9d4d into main Jan 9, 2024
6 checks passed
@fortuna fortuna deleted the fortuna-override branch January 9, 2024 02:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants