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

Customizable DNS resolver settings #625

Merged
merged 1 commit into from
Oct 15, 2015
Merged

Customizable DNS resolver settings #625

merged 1 commit into from
Oct 15, 2015

Conversation

subnetmarco
Copy link
Member

No description provided.

subnetmarco added a commit that referenced this pull request Oct 15, 2015
Customizable DNS resolver settings
@subnetmarco subnetmarco merged commit 3114ef7 into next Oct 15, 2015
@subnetmarco subnetmarco deleted the feat/custom-dns branch October 15, 2015 00:25
@thibaultcha
Copy link
Member

Nice. Can you update the new configuration branch with the proper defaults and comments in kong.yml?

@subnetmarco
Copy link
Member Author

Yep, doing it now.

@subnetmarco
Copy link
Member Author

Also updating the website.

@thibaultcha
Copy link
Member

Also I believe the less nested the properties are, the better (for un commenting)

Like:

resolver: 8.8.8.8
dnsmasq: 127.0.0.1:xxx

And resolver can either take an IP address, or "dnsmasq". Simply explain it in the comments. Then in the configuration parsing file, just put resolver the final ip address and add a flag to the config object if it is dnsmasq.

Much simpler for users, not nested.

@thibaultcha
Copy link
Member

Also dnsmasq is now an optional dependency. It should probably be removed from all the installation methods and in the comments of the config file we explain that it needs to be installed if enabled

@subnetmarco
Copy link
Member Author

Also I believe the less nested the properties are, the better (for un commenting)

In this case dnsmasq is always listening on localhost, so specifying an address for it is superfluous, while we still want to be able to specify a full address when dnsmasq is not used, hence the current structure.

Also dnsmasq is now an optional dependency. It should probably be removed from all the installation methods and in the comments of the config file we explain that it needs to be installed if enabled

Although now we offer more flexibility, dnsmasq is still the default and preferred option because with dnsmasq we will use whatever local setting has been configured for DNS resolutions (that includes /etc/hosts). Without dnsmasq Kong wouldn't be able, for example, to properly resolve localhost because that is an entry of /etc/hosts.

For this reason substantially this PR doesn't change how DNS resolution works in Kong, it just enables more flexibility.

@thibaultcha
Copy link
Member

About the first point, then just make the property dnsmasq_port. But only 2 properties, and not nested, are necessary here

@subnetmarco
Copy link
Member Author

I thought about it, but the condition here is "use either dnsmasq, or a custom address, but not both". I thought about giving a prioritization so if the address is specified, automatically skip the dnsmasq setting, but that would make the behavior a little obscure.

Having this:

resolver: 8.8.8.8
dnsmasq_port: 8053

Doesn't really make clear which one is going to be used. While this:

dns_resolver:
  address: "8.8.8.8:53"
  dnsmasq:
    enabled: false
    port: 8053

makes it more clear IMHO. Basically I decided a more verbose, but more understandable, solution over a less verbose one that would also be less clear.

@thibaultcha
Copy link
Member

Not if you explain it in the comments, it makes everything much more concise and the properties are easier to tweaks (and in comment) when they are not nested.

On Oct 14, 2015, at 5:44 PM, Marco Palladino [email protected] wrote:

I thought about it, but the condition here is "use either dnsmasq, or a custom address, but not both". I thought about giving a prioritization so if the address is specified, automatically skip the dnsmasq setting, but that would make the behavior a little obscure.

Having this:

resolver: 8.8.8.8
dnsmasq_port: 8053
Doesn't really make clear which one is going to be used. While this:

dns_resolver:
address: "8.8.8.8:53"
dnsmasq:
enabled: false
port: 8053
makes it more clear IMHO. Basically I decided a more verbose, but more understandable, solution over a less verbose one that would also be less clear.


Reply to this email directly or view it on GitHub.

@thibaultcha
Copy link
Member

Or:

dns_resolver:

/
dns_resolver_address: 8.8.8.8
dnsmasq_port: xxxx

With appropriate comments for each. Better?

On Oct 14, 2015, at 5:44 PM, Marco Palladino [email protected] wrote:

I thought about it, but the condition here is "use either dnsmasq, or a custom address, but not both". I thought about giving a prioritization so if the address is specified, automatically skip the dnsmasq setting, but that would make the behavior a little obscure.

Having this:

resolver: 8.8.8.8
dnsmasq_port: 8053
Doesn't really make clear which one is going to be used. While this:

dns_resolver:
address: "8.8.8.8:53"
dnsmasq:
enabled: false
port: 8053
makes it more clear IMHO. Basically I decided a more verbose, but more understandable, solution over a less verbose one that would also be less clear.


Reply to this email directly or view it on GitHub.

@subnetmarco
Copy link
Member Author

Another thing to consider is that the dns_resolver section could be improved in the future to add specific properties like configurable ipv6 support, and DNS cache override (nginx can override the default TTL returned by the DNS server), so it could be expanded to something like:

dns_resolver:
  ipv6: true,
  ttl: 30 # 30 seconds
  address: "8.8.8.8:53"
  dnsmasq:
    enabled: false
    port: 8053

These two properties would be effective for both custom address, or dnsmasq, so makes sense to have a parent dns_resolver section.

@subnetmarco
Copy link
Member Author

I could implement those two additional properties now too, btw.

@thibaultcha
Copy link
Member

What really bother me is how nested in the enabled property for dnsmasq. It should be at root level or at least in the dns_resolver key.

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