chore(deps) Update dns dependency and options #2625
Conversation
There was a problem hiding this comment.
It feels like we are adding a lot of configuration values that maybe feel overkill? Is there really a common use case for each one of these 5 new values, and can't a few of them be either grouped together, or be hard coded with sane values?
I expect the answer to be no for the sake of giving the user as many choices as possible (or defer responsibility to him/her), but damn, I don't remember the Nginx resolver needing that many configuration properties...
| # request will trigger its own dns query. | ||
| # When disabled multiple requests for the | ||
| # same name/type will be synchronised to a | ||
| # single query. |
There was a problem hiding this comment.
As a user, what's the benefit I get from tweaking that setting? What's the trade-off between one or multiple queries?
Or, is this new behavior not yet production tested and this value is to ensure that if something goes wrong, the user can revert back to the original behavior?
There was a problem hiding this comment.
This setting I'd love to get rid of, but it is the one that enables a query per request. (if combined with dns_stale_ttl = 0 and record ttl being 0 as well). I left the description intentionally like this, to motivate people to not use it unless instructed to.
There was a problem hiding this comment.
What I am curious about is, why would I be instructed to change this default - as in, under which circumstances? Since it seems to be the default is the most sane value and I see little (no) value in the old behavior compared to this new one (except, as mentioned, reverting to stability if something goes wrong).
kong/tools/dns.lua
Outdated
| order = conf.dns_order, -- order of trying record types | ||
| noSynchronisation = conf.dns_no_sync |
There was a problem hiding this comment.
We had a trailing comma in the previous line, and it's great since the diff was easy to read here. However, no trailing comma here 😅
kong.conf.default
Outdated
| # 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_stale_ttl = 4.0 # Once a record expires, how much longer (in |
There was a problem hiding this comment.
When a value's unit of time is in seconds, I think it makes it simpler for the user if we avoid the fractional part in the defaults. It makes things unnecessary complicated.
There was a problem hiding this comment.
the intent is to show that fractional part are accepted and valid. As with the other similar values.
There was a problem hiding this comment.
Yes, that was clearly the intent, but it feels unnecessarily cumber-stone, that's all.
kong.conf.default
Outdated
| # So stale data will be used from expiry of a | ||
| # record until either the refresh query | ||
| # completes, or the `dns_stale_ttl` number of | ||
| # seconds have passed. |
There was a problem hiding this comment.
This sentence reads weird because it starts with "So", which feels colloquial.
Stale records will still be returned when they expired,
until the refresh query completes, or the `dns_stale_ttl`
number of seconds have passed.
Considering the amount of issues we've had, I'd like to stick to them for now. Except for the |
Tieske
force-pushed
the
update-dns
branch
2 times, most recently
from
0563258 to
4217b7c
Compare
Sounds good, let's postpone this to the incoming revamp/cleanup of the config file then. |
Tieske
force-pushed
the
update-dns
branch
2 times, most recently
from
7b36454 to
92a83ec
Compare
updates the dns lib to 0.6.0, adds the new `dns_stale_ttl` and `dns_no_sync` options.
thibaultcha
added
pr/ready (but hold merge)
|
@Tieske LGTM, but if you want proper CI testing, you'll need to purge the cache on Travis which uses lua-resty-dns-client 0.5 from prior builds. |
|
Merged without the title case "Update". Thanks! |
updates the dns lib to 0.6.0, adds the new
dns_stale_ttloption.