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.

Sign up for GitHub

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

hotfix(conf) convert custom nginx conf dns_resolver to string #2386

Merged
merged 2 commits into from
Apr 15, 2017
Merged

hotfix(conf) convert custom nginx conf dns_resolver to string #2386

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
17316f9
hotfix(conf) convert custom nginx conf dns_resolver to string
bungle Apr 13, 2017
4e9eadc
fix: https://github.com/Mashape/kong/pull/2386#pullrequestreview-3273…
bungle Apr 14, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions kong/cmd/utils/prefix_handler.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,7 @@ local function compile_conf(kong_config, conf_template)
end

compile_env = pl_tablex.merge(compile_env, kong_config, true) -- union
compile_env.dns_resolver = table.concat(compile_env.dns_resolver, " ")

local post_template = pl_template.substitute(conf_template, compile_env)
return string.gsub(post_template, "(${%b{}})", function(w)
Expand Down
8 changes: 8 additions & 0 deletions spec/01-unit/03-prefix_handler_spec.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,14 @@ describe("NGINX conf compiler", function()
assert.matches("worker_connections %d+;", nginx_conf)
assert.matches("multi_accept on;", nginx_conf)
end)
it("converts dns_resolver to string", function()
local nginx_conf = prefix_handler.compile_nginx_conf({
dns_resolver = { "8.8.8.8", "8.8.4.4" }
}, [[
"resolver ${{DNS_RESOLVER}} ipv6=off;"
Copy link
Member

Choose a reason for hiding this comment

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

: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... :/

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 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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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:

  1. 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.
  2. at the same time, it feels like we do not need Nginx to create a resolver if Kong never uses it... seems wasteful
  3. (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)
  4. 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?

Copy link
Member Author

Choose a reason for hiding this comment

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

Absolutely, no need to add anything back to our templates. This is just to serve current custom template users (maybe just one).

]])
assert.matches("resolver 8.8.8.8 8.8.4.4 ipv6=off;", nginx_conf, nil, true)
end)
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps a regression test where dns_resolver is a string as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you mean something else than this:
https://github.com/Mashape/kong/blob/master/spec/01-unit/02-conf_loader_spec.lua#L240

Copy link
Member

Choose a reason for hiding this comment

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

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 :)

end)

describe("prepare_prefix()", function()
Expand Down