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.

Sign up for GitHub

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

define Resolver type #83

Closed
wants to merge 5 commits into from
+24 −0
Closed

define Resolver type #83

Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5c774e7
init
daniellacosse Oct 4, 2023
8fcde7c
Update transport/resolver.go
daniellacosse Oct 5, 2023
d8fa58d
create dns package
daniellacosse Oct 5, 2023
0997a79
oops, delete old resolver file
daniellacosse Oct 5, 2023
fd39c68
Update resolver.go
daniellacosse Oct 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions dns/resolver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
// Copyright 2023 Jigsaw Operations LLC
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// https://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package dns

/*
daniellacosse marked this conversation as resolved.
Show resolved Hide resolved
A resolver is a function that resolves a hostname to a list of IP addresses.

TODO: provide factory methods to create a Resolver. For example:

```go
daniellacosse marked this conversation as resolved.
Show resolved Hide resolved
manualDns := makeManualResolver(
daniellacosse marked this conversation as resolved.
Show resolved Hide resolved
map[string][]string{ "example.com": []string{"255.255.255.255:80" }
})

dialer := &shadowsocks.NewStreamDialer(endpoint, key, manualDns)
````
*/
type Resolver func(host string) ([]string, error)
Copy link
Contributor

Choose a reason for hiding this comment

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

A function is unmodifiable. Think about the following scenario:

dohResolver := NewDoHResolver(...)
dialer := NewDialerWithResolver(dohResolver)
// ... initially
dohResolver.SetTimeout(xxx)
// ... after some time
dohResolver.SetTimeout(yyy)

Unless we decide that all our Resolver implementations are immutable, I would prefer to define it as an interface:

type Resolver interface {
  LookupIP func(string)([]string, error)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why would they need to be modifiable? I guess I don't see a reason it shouldn't be immutable

Copy link
Contributor

@jyyi1 jyyi1 Oct 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

For example Intra allows to you change the DoH server on the fly (without reconnecting).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And switching out the one resolver function with another wouldn't work in that case?

Copy link
Contributor

@jyyi1 jyyi1 Oct 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Sure it does. But it also means we will create a new interface in the app that wraps around this func.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we? Why can't it be a property on an existing struct?

Copy link
Contributor

@jyyi1 jyyi1 Oct 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Well yes, defining a struct that wraps around the function, which is OK for a single app.

My concern is whether a "stateful" Resolver is common across all apps, and how it interacts with StreamDialer and PacketProxy.

For example, some implementation might maintain a connection to a remote server, then we need to have Close() function in the Resolver to prevent resource leak.

We can discuss them in the brainstorm meeting, it all depends on the use cases we would like to cover. We might need to consider more stuffs than the LocalResolver or shadowsocks.StreamDialer.

Another thing I can think of is the two Resolve methods, one with context.Context and one without (similar to Go Dialer's Dial and DialContext)? We can discuss whether we should support it too.