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

Consider exposing DnsNameResolverBuilder or DnsNameResolverBuilder#datagramChannelStrategy #6122

Open
jrhee17 opened this issue Feb 25, 2025 · 6 comments · May be fixed by #6127
Open

Consider exposing DnsNameResolverBuilder or DnsNameResolverBuilder#datagramChannelStrategy #6122

jrhee17 opened this issue Feb 25, 2025 · 6 comments · May be fixed by #6127
Milestone

Comments

@jrhee17
Copy link
Contributor

jrhee17 commented Feb 25, 2025

Motivation

We received a report where the nameserver address isn't being updated.

netty/netty#14382
netty/netty#14364

A possible cause could be that the channel is cached per resolver by default, which may not reflect nameserver updates correctly.

Proposal
I propose that we expose the DnsNameResolverBuilder#datagramChannelStrategy as well as additional useful options in AbstractDnsResolverBuilder.
Alternatively, I think we could also expose the DnsNameResolverBuilder directly so that users can modify specific settings with more freedom.

ref: https://discord.com/channels/1087271586832318494/1341789100000415817/1341789105461264384

@jrhee17 jrhee17 added this to the 1.32.0 milestone Feb 25, 2025
@ikhoon
Copy link
Contributor

ikhoon commented Feb 25, 2025

DnsNameResolverBuilder directly so that users can modify specific settings with more freedom

Are you considering exposing Consumer<DnsNameResolverBuilder> after setting Armeria’s default configurations?

// Disable all caches provided by Netty and use DnsCache instead.
.resolveCache(NoopDnsCache.INSTANCE)
.authoritativeDnsServerCache(NoopAuthoritativeDnsServerCache.INSTANCE)
.cnameCache(NoopDnsCnameCache.INSTANCE)
.traceEnabled(traceEnabled)
.completeOncePreferredResolved(true)
.recursionDesired(recursionDesired)
.maxQueriesPerResolve(maxQueriesPerResolve)
.maxPayloadSize(maxPayloadSize)
.optResourceEnabled(optResourceEnabled)
// Disable DnsNameResolver from resolving host files and use HostsFileDnsResolver instead.
.hostsFileEntriesResolver(NoopHostFileEntriesResolver.INSTANCE)
.nameServerProvider(serverAddressStreamProvider)

@jrhee17
Copy link
Contributor Author

jrhee17 commented Feb 25, 2025

Are you considering exposing Consumer after setting Armeria’s default configurations?

Right, similar to how we are already exposing GraphQlServiceBuilder#runtimeWiring or ServerBuilder#childChannelPipelineCustomizer.

Alternatively, I think we can also expose Consumer<DnsNameResolverBuilder> in a less exposed location (e.g. Flags) where the default DnsNameResolverBuilder is applied first, and then resolver-specific configurations are applied like in the snippet you shared.

@ikhoon
Copy link
Contributor

ikhoon commented Feb 25, 2025

Continuously updating the API might be painful. However, the Netty API hasn’t changed frequently recently and there’s already a delegated builder for DNS resolver. So I’m slightly leaning toward adding setters to DnsResolverGroupBuilder for now.

How about considering introducing a customizer if a similar issue arises again in the future?

@jrhee17
Copy link
Contributor Author

jrhee17 commented Feb 25, 2025

Alternatively, I think we can also expose Consumer in a less exposed location (e.g. Flags) where the default DnsNameResolverBuilder is applied first

We could also introduce a new MoreFlags or ExperimentalFlags as opposed to Flags

@berendjan
Copy link

berendjan commented Feb 25, 2025

Im also looking at this exact issue, was hoping that the Builder could be exposed. Thanks for looking at this issue!

@jrhee17
Copy link
Contributor Author

jrhee17 commented Feb 26, 2025

It seems like the other maintainers prefer that DnsNameResolverBuilder is not exposed directly on the grounds of:

  • DnsNameResolverBuilder does not seem to change very often
  • To avoid breaking changes due to third party libraries

Let me just expose datagramChannelStrategy, socketChannel* for this iteration

@jrhee17 jrhee17 linked a pull request Feb 26, 2025 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants