hotfix(conf) convert custom nginx conf dns_resolver to string #2386
Conversation
If you have this in your custom nginx template:
```nginx
resolver ${{DNS_RESOLVER}} ipv6=off;
```
It will yield an error. This PR fixes the issue by transforming
the `dns_resolver` from `table` to `string` for template usage.
Fix #2319
| "resolver ${{DNS_RESOLVER}} ipv6=off;" | ||
| ]]) | ||
| assert.matches("resolver 8.8.8.8 8.8.4.4 ipv6=off;", nginx_conf, nil, true) | ||
| end) |
There was a problem hiding this comment.
Perhaps a regression test where dns_resolver is a string as well?
There was a problem hiding this comment.
Do you mean something else than this:
https://github.com/Mashape/kong/blob/master/spec/01-unit/02-conf_loader_spec.lua#L240
There was a problem hiding this comment.
Perhaps a regression test where dns_resolver is a string as well?
Such a test exists here for all array properties and not just dns_resolver :)
kong/cmd/utils/prefix_handler.lua
Outdated
| @@ -180,6 +180,10 @@ local function compile_conf(kong_config, conf_template) | |||
|
|
|||
| compile_env = pl_tablex.merge(compile_env, kong_config, true) -- union | |||
|
|
|||
| if type(compile_env.dns_resolver) == "table" then | |||
There was a problem hiding this comment.
I think this value always is a table?
There was a problem hiding this comment.
I think we can merge this patch if we get rid of this if :)
| local nginx_conf = prefix_handler.compile_nginx_conf({ | ||
| dns_resolver = { "8.8.8.8", "8.8.4.4" } | ||
| }, [[ | ||
| "resolver ${{DNS_RESOLVER}} ipv6=off;" |
There was a problem hiding this comment.
:O This is a bit concerning. Apparently this is not in the template anymore, and maybe the user who reported it is using an old version of the template? We should check with him, and probably add it back... :/
There was a problem hiding this comment.
Yes, it is not. Is it problem to have that in case user wants to add a resolver line in his template? This will not break anything in case there is no resolver line in a template (our dns resolver uses this). We could also suggest user adding for loop in his template, but it would be a bit ugly.
There was a problem hiding this comment.
I mean that if user wants the Nginx resolver use the same resolvers as Kong DNS resolver and configure it in one place, does it hurt?
There was a problem hiding this comment.
Hmmm at the same time, it is true we do not need it anymore since all of our DNS resolution is currently taken care of on the Lua-land...
I mean that if user wants the Nginx resolver use the same resolvers as Kong DNS resolve
I feel like this is a completely valid point, and to sum up the concerns, I would say they are:
- some users may indeed want to avoid duplicating their DNS nameservers in both the Kong config and a custom Nginx template, if they need to.
- at the same time, it feels like we do not need Nginx to create a resolver if Kong never uses it... seems wasteful
- (on the long term) I am a bit concerned by the fact that we wrap each cosocket
:connect()call with our own Lua-land resolver. While it is nice to be able to use the same DNS resolver everywhere and feed it with better features than the Nginx one, I am curious about the performance impact of that, and wonder if we should not instead invest time and energy in the Nginx resolver itself (a long-time thought of mine) - this change makes our current configuration types compatible with legacy Nginx templates, such as the one in use by the user who raised this issue
Because of 2. and 4. (probably most of the use-cases), I think it's fine to merge this patch as-is, without adding it back to the template are I suggested to maybe do.
Do you agree with this reasoning?
There was a problem hiding this comment.
Absolutely, no need to add anything back to our templates. This is just to serve current custom template users (maybe just one).
thibaultcha
added
pr/changes requested
bungle
added
pr/status/please review
and removed
pr/changes requested
|
Thanks! |
Summary
If you have this in your custom nginx template:
resolver ${{DNS_RESOLVER}} ipv6=off;It will yield an error. This PR fixes the issue by transforming
the
dns_resolverfromtabletostringfor template usage.Issues resolved
Fix #2319