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(runtime): add optional custom dns resolver to worker options #22293

Closed
wants to merge 15 commits into from

Conversation

primoze
Copy link

@primoze primoze commented Feb 6, 2024

This implements #17165.

It allows specifying a custom DNS resolver in WorkerOptions (and WebWorkerOptions), which is then used when creating Options for the fetch API, and HttpOptions for the KV remote DB handler.

Supersedes #17166, as it seems abandoned (and deno has changed a lot since then).

I changed the deno_fetch::create_http_client signature, because adding the resolver to CreateHttpClientOptions would require a + Debug trait bound.

cli/Cargo.toml Outdated
@@ -155,6 +155,7 @@ http.workspace = true
http-body-util.workspace = true
hyper.workspace = true
hyper-util.workspace = true
hyper_v014.workspace = true
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't need to add this dep -- if we need anything from hyper_v014, we should re-export those bits from ext/fetch.

Copy link
Author

Choose a reason for hiding this comment

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

Just exported Name from deno_fetch. Should it be in a dns module or something, so that it indicate it relates to DNS?

cli/file_fetcher.rs Outdated Show resolved Hide resolved
ext/kv/remote.rs Outdated
@@ -127,6 +129,7 @@ impl<P: RemoteDbHandlerPermissions + 'static> DatabaseHandler
let options = &self.http_options;
let client = create_http_client(
&options.user_agent,
options.dns_resolver.clone(),
Copy link
Contributor

@mmastrac mmastrac Feb 23, 2024

Choose a reason for hiding this comment

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

On second thought -- maybe this should live on CreateHttpClientOptions?

I think it's fine to require a debug bound on the resolver. If we run into trouble with that, we should create our own intermediate trait and export that from ext/fetch instead.

Copy link
Author

Choose a reason for hiding this comment

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

I went with a struct instead of a trait (DnsResolver), and implemented Debug. A trait seemed kind of overkill, idk. Wdyt?

_name: hyper_v014::client::connect::dns::Name,
) -> Resolving {
let iter: Box<dyn Iterator<Item = SocketAddr> + Send> = Box::new(
vec!["127.0.0.1:0".parse::<SocketAddr>().unwrap()].into_iter(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Just return an Ipv4Addr::LOCALHOST here

cli/file_fetcher.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@mmastrac mmastrac left a comment

Choose a reason for hiding this comment

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

Looks good, but just a few mechanical changes before we can land it.

@primoze primoze requested a review from mmastrac April 3, 2024 12:37
@bartlomieju
Copy link
Member

@primoze sorry for a slow response. Would #26740 solve your issue or do you need to have totally custom resolver? If so I suggest to open a new PR as this one is gonna be a nightmare to rebase.

@bartlomieju
Copy link
Member

I'm going to close this one for now. Please ping me if you'd like to revive it.

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.

3 participants