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

allow ipv6 nameservers #2634

Merged
merged 2 commits into from
Jun 21, 2017
Merged

allow ipv6 nameservers #2634

merged 2 commits into from
Jun 21, 2017

Conversation

Tieske
Copy link
Member

@Tieske Tieske commented Jun 19, 2017

allows the nameservers to be configured in both ipv4 and ipv6 format. And if the /etc/resolv.conf file contains ipv6 nameserver entries, they will be used as well.

Fixes the "Error: Invalid configuration, no dns servers found" error on systems with only ipv6 nameservers configured.

(this builds on top of PR #2625 )

@Tieske Tieske force-pushed the fix/ipv6-nameservers branch from 8928eb9 to 9c4d7e9 Compare June 19, 2017 10:06
# completes, or the `dns_stale_ttl` number of
# seconds have passed.

#dns_not_found_ttl = 30 # Ttl in seconds for empty DNS responses and
Copy link
Contributor

Choose a reason for hiding this comment

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

"ttl" should either be all caps or all lowercase (prefer upper here)

@@ -69,8 +69,10 @@ db_cache_ttl = 3600
dns_resolver = NONE
dns_hostsfile = /etc/hosts
dns_order = LAST,SRV,A,CNAME
dns_stale_ttl = 4.0
Copy link
Contributor

Choose a reason for hiding this comment

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

should be 4, not 4.0, to match the default config file

end
end

local opts = {
hosts = conf.dns_hostsfile,
resolvConf = nil, -- defaults to system resolv.conf
nameservers = servers, -- provided list or taken from resolv.conf
enable_ipv6 = true, -- allow for ipv6 nameserver addresses
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a comment on the lua-resty-dns-client lib, not this PR per se, but this mix of snake_case and camelCase variables is really rough.

Copy link
Member

Choose a reason for hiding this comment

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

Wanted to make the same comment but now consistently afraid of being called too pedantic or detail oriented :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it absolutely needs a cleanup. I'll try to find some time before the next breaking change, so it can come along then.

#dns_stale_ttl = 4 # Once a record expires, how much longer (in
# seconds) may the current stale record be
# used while the refresh query is executed in
# the background.
Copy link
Contributor

Choose a reason for hiding this comment

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

"Defines, in seconds, how long a record will remain in cache past its TTL. This value will be used while the new DNS record is fetched in the background." ?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes. So while the record is valid (within ttl) it is just being used. Once it expires, (stale) it will trigger a background re-query. Once that requery completes, the record is valid again. So this setting is the maximum time that a record will be used while being stale. When it is refreshed it is no longer stale, and when it doesn't refresh within this stale ttl time, then requests will no longer use the stale record (it will effectively be deleted by then from the cache), but will synchronously wait for completion of the dns query (which is similar to the very first query when there is no stale record yet, and requests will have to wait for the query to finish as well)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes :) I was suggesting a re-word here, as its phrase awkwardly. My fault for not clarifying the meaning of this comment.

Copy link
Member

Choose a reason for hiding this comment

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

I think what both @p0pr0ck5 and my comment were trying to say, is that both sentences in this description read a bit awkwardly, and could be phrased better while giving more information to the user.

Copy link
Contributor

Choose a reason for hiding this comment

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

(If there are no other blockers for this merge I'd be happy to clean up the language and push).

Copy link
Member Author

Choose a reason for hiding this comment

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

duh...

@p0pr0ck5 p0pr0ck5 added the pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. label Jun 19, 2017
updates the dns lib to 0.6.0, adds the new `dns_stale_ttl` and
`dns_no_sync` options.
@Tieske Tieske force-pushed the fix/ipv6-nameservers branch from 9c4d7e9 to c230174 Compare June 19, 2017 18:23
# record types. The `LAST` type means the
# type of the last successful lookup (for the
# specified name). The format is a (case
# insensitive) comma separated list.

#dns_not_found_ttl = 30.0 # ttl in seconds for empty DNS responses and
<<<<<<< HEAD
Copy link
Member

Choose a reason for hiding this comment

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

something went wrong

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@Tieske Tieske force-pushed the fix/ipv6-nameservers branch from c230174 to 43d65b8 Compare June 19, 2017 18:35
@Tieske Tieske added pr/status/please review and removed pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. labels Jun 20, 2017
@Tieske Tieske added this to the 0.11.0 milestone Jun 20, 2017
@p0pr0ck5 p0pr0ck5 merged commit 04e5127 into next Jun 21, 2017
@p0pr0ck5 p0pr0ck5 deleted the fix/ipv6-nameservers branch June 21, 2017 15:41
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