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

Add new feature trust-dns to lib & bin #318

Merged
merged 5 commits into from
Aug 25, 2022
Merged

Add new feature trust-dns to lib & bin #318

merged 5 commits into from
Aug 25, 2022

Conversation

NobodyXu
Copy link
Member

Signed-off-by: Jiahao XU [email protected]

@NobodyXu NobodyXu marked this pull request as ready for review August 24, 2022 08:39
@NobodyXu NobodyXu requested a review from passcod August 24, 2022 08:39
@NobodyXu NobodyXu added the PR: feature work PR that provides or works on a new feature label Aug 24, 2022
@NobodyXu NobodyXu enabled auto-merge (squash) August 24, 2022 08:45
@NobodyXu
Copy link
Member Author

I enabled trust-dns feature on musl builds because its resolver does not support TCP and might also miss other features since it is quite compact.

@NobodyXu NobodyXu disabled auto-merge August 24, 2022 09:05
@NobodyXu NobodyXu enabled auto-merge (squash) August 24, 2022 09:05
@NobodyXu
Copy link
Member Author

I've started a release build for this PR https://github.com/cargo-bins/cargo-binstall/actions/runs/2917842935 to see how it affects binary size.

@passcod
Copy link
Member

passcod commented Aug 24, 2022

If it's okay size wise we should just enable it by default, proper DNS resolution is the right thing to do, especially given the various issues with getaddrinfo

@NobodyXu
Copy link
Member Author

If it's okay size wise we should just enable it by default, proper DNS resolution is the right thing to do, especially given the various issues with getaddrinfo

I've run the CI on both main and this PR.

The result is roughly 0.5M increase.

P.S. it seems that the past PRs have caused the bin size to increase by 1-2M, might be worth investigation.

Because the `getaddrinfo` is a poorly designed API.

Signed-off-by: Jiahao XU <[email protected]>
@NobodyXu NobodyXu disabled auto-merge August 24, 2022 10:33
@NobodyXu
Copy link
Member Author

If it's okay size wise we should just enable it by default, proper DNS resolution is the right thing to do, especially given the various issues with getaddrinfo

Given that trust-dns only increase the binary size by 0.5M, I've enabled it by default.
Now we should have dnssec support using ring and dns over tls support for all targets.

Though trust-dns only supports dns over https if rustls is used, thus the binaries built for windows does not have
dns over https since the CI enables native tls feature for windows.

In the future, trust-dns will also support dns over quic (http3), which also only supports rustls.

@passcod Shall we enable rustls feature on windows?

@passcod
Copy link
Member

passcod commented Aug 24, 2022

I think so yes, that will also allow us to have consistent secure transport rules e.g. wintls not supporting 1.3 on some windows versions

because wintls on the CI does not support TLS 1.3 and trust-dns only
support dns over https if rustls is used.

Signed-off-by: Jiahao XU <[email protected]>
@NobodyXu
Copy link
Member Author

I think so yes, that will also allow us to have consistent secure transport rules e.g. wintls not supporting 1.3 on some windows versions

Done!

@NobodyXu NobodyXu enabled auto-merge (squash) August 24, 2022 10:43
@NobodyXu
Copy link
Member Author

The result is roughly 0.5M increase.

Note that this is before compression.

P.S. it seems that the past PRs have caused the bin size to increase by 1-2M, might be worth investigation.

I was comparing size before compression vs size after compression.

@passcod
Copy link
Member

passcod commented Aug 25, 2022

Hmm well, I think it's worth it for the feature set it adds.

@NobodyXu NobodyXu merged commit 4def4d0 into main Aug 25, 2022
@NobodyXu NobodyXu deleted the trust-dns branch August 25, 2022 00:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: feature work PR that provides or works on a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants