From d46b4148fa193a3661ad86032be6a7e0cbde5faf Mon Sep 17 00:00:00 2001 From: Elvin Efendi Date: Tue, 13 Aug 2019 18:21:22 -0400 Subject: [PATCH] Lua /etc/resolv.conf parser and some refactoring --- build/dev-env.sh | 2 + .../ingress/controller/template/template.go | 32 -------- .../controller/template/template_test.go | 37 --------- rootfs/etc/nginx/lua/configuration.lua | 4 +- rootfs/etc/nginx/lua/test/balancer_test.lua | 3 +- .../etc/nginx/lua/test/configuration_test.lua | 5 ++ rootfs/etc/nginx/lua/test/dns_helper.lua | 27 ------- rootfs/etc/nginx/lua/test/helpers.lua | 47 +++++++++++ rootfs/etc/nginx/lua/test/run.lua | 3 + rootfs/etc/nginx/lua/test/util/dns_test.lua | 30 ++++--- .../nginx/lua/test/util/resolv_conf_test.lua | 64 +++++++++++++++ rootfs/etc/nginx/lua/util/dns.lua | 5 +- rootfs/etc/nginx/lua/util/resolv_conf.lua | 79 +++++++++++++++++++ rootfs/etc/nginx/template/nginx.tmpl | 2 - test/e2e/lua/dynamic_configuration.go | 8 -- 15 files changed, 224 insertions(+), 124 deletions(-) delete mode 100644 rootfs/etc/nginx/lua/test/dns_helper.lua create mode 100644 rootfs/etc/nginx/lua/test/helpers.lua create mode 100644 rootfs/etc/nginx/lua/test/util/resolv_conf_test.lua create mode 100644 rootfs/etc/nginx/lua/util/resolv_conf.lua diff --git a/build/dev-env.sh b/build/dev-env.sh index a4f1aee6d..c45cd78cd 100755 --- a/build/dev-env.sh +++ b/build/dev-env.sh @@ -56,6 +56,8 @@ if ! kubectl get namespace "${NAMESPACE}"; then kubectl create namespace "${NAMESPACE}" fi +kubectl get deploy nginx-ingress-controller -n ${NAMESPACE} && kubectl delete deploy nginx-ingress-controller -n ${NAMESPACE} + ROOT=./deploy/minikube if [[ ${KUBE_CLIENT_VERSION} -lt 14 ]]; then diff --git a/internal/ingress/controller/template/template.go b/internal/ingress/controller/template/template.go index fa09c9fd8..60bdab0ca 100644 --- a/internal/ingress/controller/template/template.go +++ b/internal/ingress/controller/template/template.go @@ -144,7 +144,6 @@ var ( "filterRateLimits": filterRateLimits, "buildRateLimitZones": buildRateLimitZones, "buildRateLimit": buildRateLimit, - "buildResolversForLua": buildResolversForLua, "configForLua": configForLua, "locationConfigForLua": locationConfigForLua, "buildResolvers": buildResolvers, @@ -279,37 +278,6 @@ func buildLuaSharedDictionaries(c interface{}, s interface{}, disableLuaRestyWAF return strings.Join(out, ";\n\r") + ";" } -func buildResolversForLua(res interface{}, disableIpv6 interface{}) string { - nss, ok := res.([]net.IP) - if !ok { - klog.Errorf("expected a '[]net.IP' type but %T was returned", res) - return "" - } - no6, ok := disableIpv6.(bool) - if !ok { - klog.Errorf("expected a 'bool' type but %T was returned", disableIpv6) - return "" - } - - if len(nss) == 0 { - return "" - } - - r := []string{} - for _, ns := range nss { - if ing_net.IsIPV6(ns) { - if no6 { - continue - } - r = append(r, fmt.Sprintf("\"[%v]\"", ns)) - } else { - r = append(r, fmt.Sprintf("\"%v\"", ns)) - } - } - - return strings.Join(r, ", ") -} - // configForLua returns some general configuration as Lua table represented as string func configForLua(input interface{}) string { all, ok := input.(config.TemplateConfig) diff --git a/internal/ingress/controller/template/template_test.go b/internal/ingress/controller/template/template_test.go index ef8f4ccdd..d79ae6d4c 100644 --- a/internal/ingress/controller/template/template_test.go +++ b/internal/ingress/controller/template/template_test.go @@ -577,43 +577,6 @@ func TestBuildForwardedFor(t *testing.T) { } } -func TestBuildResolversForLua(t *testing.T) { - - ipOne := net.ParseIP("192.0.0.1") - ipTwo := net.ParseIP("2001:db8:1234:0000:0000:0000:0000:0000") - ipList := []net.IP{ipOne, ipTwo} - - invalidType := &ingress.Ingress{} - expected := "" - actual := buildResolversForLua(invalidType, false) - - // Invalid Type for []net.IP - if expected != actual { - t.Errorf("Expected '%v' but returned '%v'", expected, actual) - } - - actual = buildResolversForLua(ipList, invalidType) - - // Invalid Type for bool - if expected != actual { - t.Errorf("Expected '%v' but returned '%v'", expected, actual) - } - - expected = "\"192.0.0.1\", \"[2001:db8:1234::]\"" - actual = buildResolversForLua(ipList, false) - - if expected != actual { - t.Errorf("Expected '%v' but returned '%v'", expected, actual) - } - - expected = "\"192.0.0.1\"" - actual = buildResolversForLua(ipList, true) - - if expected != actual { - t.Errorf("Expected '%v' but returned '%v'", expected, actual) - } -} - func TestBuildResolvers(t *testing.T) { ipOne := net.ParseIP("192.0.0.1") ipTwo := net.ParseIP("2001:db8:1234:0000:0000:0000:0000:0000") diff --git a/rootfs/etc/nginx/lua/configuration.lua b/rootfs/etc/nginx/lua/configuration.lua index 48d61d83d..b32cbf507 100644 --- a/rootfs/etc/nginx/lua/configuration.lua +++ b/rootfs/etc/nginx/lua/configuration.lua @@ -4,9 +4,7 @@ local cjson = require("cjson.safe") local configuration_data = ngx.shared.configuration_data local certificate_data = ngx.shared.certificate_data -local _M = { - nameservers = {} -} +local _M = {} function _M.get_backends_data() return configuration_data:get("backends") diff --git a/rootfs/etc/nginx/lua/test/balancer_test.lua b/rootfs/etc/nginx/lua/test/balancer_test.lua index 0bd1cddcf..8c9d72ed5 100644 --- a/rootfs/etc/nginx/lua/test/balancer_test.lua +++ b/rootfs/etc/nginx/lua/test/balancer_test.lua @@ -315,8 +315,7 @@ describe("Balancer", function() } } - local dns_helper = require("test/dns_helper") - dns_helper.mock_dns_query({ + helpers.mock_resty_dns_query({ { name = "example.com", address = "192.168.1.1", diff --git a/rootfs/etc/nginx/lua/test/configuration_test.lua b/rootfs/etc/nginx/lua/test/configuration_test.lua index d5db0844a..54085d03d 100644 --- a/rootfs/etc/nginx/lua/test/configuration_test.lua +++ b/rootfs/etc/nginx/lua/test/configuration_test.lua @@ -115,15 +115,20 @@ describe("Configuration", function() end) it("returns a status of 400", function() + local original_io_open = _G.io.open _G.io.open = function(filename, extension) return false end assert.has_no.errors(configuration.call) assert.equal(ngx.status, ngx.HTTP_BAD_REQUEST) + _G.io.open = original_io_open end) it("logs 'dynamic-configuration: unable to read valid request body to stderr'", function() + local original_io_open = _G.io.open + _G.io.open = function(filename, extension) return false end local s = spy.on(ngx, "log") assert.has_no.errors(configuration.call) assert.spy(s).was_called_with(ngx.ERR, "dynamic-configuration: unable to read valid request body") + _G.io.open = original_io_open end) end) diff --git a/rootfs/etc/nginx/lua/test/dns_helper.lua b/rootfs/etc/nginx/lua/test/dns_helper.lua deleted file mode 100644 index 570a60e52..000000000 --- a/rootfs/etc/nginx/lua/test/dns_helper.lua +++ /dev/null @@ -1,27 +0,0 @@ -local _M = {} - -local configuration = require("configuration") -local resolver = require("resty.dns.resolver") -local old_resolver_new = resolver.new - -local function reset(nameservers) - configuration.nameservers = nameservers or { "1.1.1.1" } -end - -function _M.mock_new(func, nameservers) - reset(nameservers) - resolver.new = func -end - -function _M.mock_dns_query(response, err) - reset() - resolver.new = function(self, options) - local r = old_resolver_new(self, options) - r.query = function(self, name, options, tries) - return response, err - end - return r - end -end - -return _M diff --git a/rootfs/etc/nginx/lua/test/helpers.lua b/rootfs/etc/nginx/lua/test/helpers.lua new file mode 100644 index 000000000..e115e528a --- /dev/null +++ b/rootfs/etc/nginx/lua/test/helpers.lua @@ -0,0 +1,47 @@ +local _M = {} + +local resty_dns_resolver = require("resty.dns.resolver") + +local original_resty_dns_resolver_new = resty_dns_resolver.new +local original_io_open = io.open + +function _M.with_resolv_conf(content, func) + local new_resolv_conf_f = assert(io.tmpfile()) + new_resolv_conf_f:write(content) + new_resolv_conf_f:seek("set", 0) + + io.open = function(path, mode) + if path ~= "/etc/resolv.conf" then + error("expected '/etc/resolv.conf' as path but got: " .. tostring(path)) + end + if mode ~= "r" then + error("expected 'r' as mode but got: " .. tostring(mode)) + end + + return new_resolv_conf_f, nil + end + + func() + + io.open = original_io_open + + if io.type(new_resolv_conf_f) ~= "closed file" then + error("file was left open") + end +end + +function _M.mock_resty_dns_new(func) + resty_dns_resolver.new = func +end + +function _M.mock_resty_dns_query(response, err) + resty_dns_resolver.new = function(self, options) + local r = original_resty_dns_resolver_new(self, options) + r.query = function(self, name, options, tries) + return response, err + end + return r + end +end + +return _M diff --git a/rootfs/etc/nginx/lua/test/run.lua b/rootfs/etc/nginx/lua/test/run.lua index 151c646a5..3836dd56e 100644 --- a/rootfs/etc/nginx/lua/test/run.lua +++ b/rootfs/etc/nginx/lua/test/run.lua @@ -11,6 +11,7 @@ do -- if there's more constants need to be whitelisted for test runs, add here. local GLOBALS_ALLOWED_IN_TEST = { _TEST = true, + helpers = true, } local newindex = function(table, key, value) rawset(table, key, value) @@ -33,6 +34,8 @@ do setmetatable(_G, { __newindex = newindex }) end +_G.helpers = require("test.helpers") + local ffi = require("ffi") local lua_ingress = require("lua_ingress") diff --git a/rootfs/etc/nginx/lua/test/util/dns_test.lua b/rootfs/etc/nginx/lua/test/util/dns_test.lua index 48e7c6ca3..1895fcbbe 100644 --- a/rootfs/etc/nginx/lua/test/util/dns_test.lua +++ b/rootfs/etc/nginx/lua/test/util/dns_test.lua @@ -1,41 +1,51 @@ +local conf = [===[ +nameserver 1.2.3.4 +nameserver 4.5.6.7 +search ingress-nginx.svc.cluster.local svc.cluster.local cluster.local +options ndots:5 +]===] + +helpers.with_resolv_conf(conf, function() + require("util.resolv_conf") +end) + describe("resolve", function() local dns = require("util.dns") - local dns_helper = require("test/dns_helper") it("sets correct nameservers", function() - dns_helper.mock_new(function(self, options) + helpers.mock_resty_dns_new(function(self, options) assert.are.same({ nameservers = { "1.2.3.4", "4.5.6.7" }, retrans = 5, timeout = 2000 }, options) return nil, "" - end, { "1.2.3.4", "4.5.6.7" }) + end) dns.resolve("example.com") end) it("returns host when an error happens", function() local s_ngx_log = spy.on(ngx, "log") - dns_helper.mock_new(function(...) return nil, "an error" end) + helpers.mock_resty_dns_new(function(...) return nil, "an error" end) assert.are.same({ "example.com" }, dns.resolve("example.com")) assert.spy(s_ngx_log).was_called_with(ngx.ERR, "failed to instantiate the resolver: an error") - dns_helper.mock_dns_query(nil, "oops!") + helpers.mock_resty_dns_query(nil, "oops!") assert.are.same({ "example.com" }, dns.resolve("example.com")) assert.spy(s_ngx_log).was_called_with(ngx.ERR, "failed to query the DNS server:\noops!\noops!") - dns_helper.mock_dns_query({ errcode = 1, errstr = "format error" }) + helpers.mock_resty_dns_query({ errcode = 1, errstr = "format error" }) assert.are.same({ "example.com" }, dns.resolve("example.com")) assert.spy(s_ngx_log).was_called_with(ngx.ERR, "failed to query the DNS server:\nserver returned error code: 1: format error\nserver returned error code: 1: format error") - dns_helper.mock_dns_query({}) + helpers.mock_resty_dns_query({}) assert.are.same({ "example.com" }, dns.resolve("example.com")) assert.spy(s_ngx_log).was_called_with(ngx.ERR, "failed to query the DNS server:\nno record resolved\nno record resolved") - dns_helper.mock_dns_query({ { name = "example.com", cname = "sub.example.com", ttl = 60 } }) + helpers.mock_resty_dns_query({ { name = "example.com", cname = "sub.example.com", ttl = 60 } }) assert.are.same({ "example.com" }, dns.resolve("example.com")) assert.spy(s_ngx_log).was_called_with(ngx.ERR, "failed to query the DNS server:\nno record resolved\nno record resolved") end) it("resolves all A records of given host, caches them with minimal ttl and returns from cache next time", function() - dns_helper.mock_dns_query({ + helpers.mock_resty_dns_query({ { name = "example.com", address = "192.168.1.1", @@ -66,7 +76,7 @@ describe("resolve", function() assert.are.same({ "192.168.1.1", "1.2.3.4" }, dns.resolve("example.com")) - dns_helper.mock_new(function(...) + helpers.mock_resty_dns_new(function(...) error("expected to short-circuit and return response from cache") end) assert.are.same({ "192.168.1.1", "1.2.3.4" }, dns.resolve("example.com")) diff --git a/rootfs/etc/nginx/lua/test/util/resolv_conf_test.lua b/rootfs/etc/nginx/lua/test/util/resolv_conf_test.lua new file mode 100644 index 000000000..b8c706d1b --- /dev/null +++ b/rootfs/etc/nginx/lua/test/util/resolv_conf_test.lua @@ -0,0 +1,64 @@ +local original_io_open = io.open + +describe("resolv_conf", function() + after_each(function() + package.loaded["util.resolv_conf"] = nil + io.open = original_io_open + end) + + it("errors when file can not be opened", function() + io.open = function(...) + return nil, "file does not exist" + end + + assert.has_error(function() require("util.resolv_conf") end, "could not open /etc/resolv.conf: file does not exist") + end) + + it("opens '/etc/resolv.conf' with mode 'r'", function() + io.open = function(path, mode) + assert.are.same("/etc/resolv.conf", path) + assert.are.same("r", mode) + + return original_io_open(path, mode) + end + + assert.has_no.errors(function() require("util.resolv_conf") end) + end) + + it("correctly parses resolv.conf", function() + local conf = [===[ +# This is a comment +nameserver 10.96.0.10 +nameserver 10.96.0.99 +search ingress-nginx.svc.cluster.local svc.cluster.local cluster.local +options ndots:5 + ]===] + + helpers.with_resolv_conf(conf, function() + local resolv_conf = require("util.resolv_conf") + assert.are.same({ + nameservers = { "10.96.0.10", "10.96.0.99" }, + search = { "ingress-nginx.svc.cluster.local", "svc.cluster.local", "cluster.local" }, + ndots = 5, + }, resolv_conf) + end) + end) + + it("ignores options that it does not understand", function() + local conf = [===[ +nameserver 10.96.0.10 +search example.com +options debug +options ndots:3 + ]===] + + helpers.with_resolv_conf(conf, function() + local resolv_conf = require("util.resolv_conf") + assert.are.same({ + nameservers = { "10.96.0.10" }, + search = { "example.com" }, + ndots = 3, + }, resolv_conf) + end) + end) +end) diff --git a/rootfs/etc/nginx/lua/util/dns.lua b/rootfs/etc/nginx/lua/util/dns.lua index fcebeae0b..94023fa1a 100644 --- a/rootfs/etc/nginx/lua/util/dns.lua +++ b/rootfs/etc/nginx/lua/util/dns.lua @@ -1,7 +1,6 @@ local resolver = require("resty.dns.resolver") local lrucache = require("resty.lrucache") -local configuration = require("configuration") -local util = require("util") +local resolv_conf = require("util.resolv_conf") local _M = {} local CACHE_SIZE = 10000 @@ -59,7 +58,7 @@ function _M.resolve(host) local r r, err = resolver:new{ - nameservers = util.deepcopy(configuration.nameservers), + nameservers = resolv_conf.nameservers, retrans = 5, timeout = 2000, -- 2 sec } diff --git a/rootfs/etc/nginx/lua/util/resolv_conf.lua b/rootfs/etc/nginx/lua/util/resolv_conf.lua new file mode 100644 index 000000000..75fb48f3f --- /dev/null +++ b/rootfs/etc/nginx/lua/util/resolv_conf.lua @@ -0,0 +1,79 @@ +local ngx_re_split = require("ngx.re").split + +local ngx_log = ngx.log +local ngx_ERR = ngx.ERR + +local CONF_PATH = "/etc/resolv.conf" + +local nameservers, search, ndots = {}, {}, 1 + +local function set_search(parts) + local length = #parts + + for i = 2, length, 1 do + search[i-1] = parts[i] + end +end + +local function set_ndots(parts) + local option = parts[2] + if not option then + return + end + + local option_parts, err = ngx_re_split(option, ":") + if err then + ngx_log(ngx_ERR, err) + return + end + + if option_parts[1] ~= "ndots" then + return + end + + ndots = tonumber(option_parts[2]) +end + +local function is_comment(line) + return line:sub(1, 1) == "#" +end + +local function parse_line(line) + if is_comment(line) then + return + end + + local parts, err = ngx_re_split(line, "\\s+") + if err then + ngx_log(ngx_ERR, err) + end + + local keyword, value = parts[1], parts[2] + + if keyword == "nameserver" then + nameservers[#nameservers + 1] = value + elseif keyword == "search" then + set_search(parts) + elseif keyword == "options" then + set_ndots(parts) + end +end + +do + local f, err = io.open(CONF_PATH, "r") + if not f then + error("could not open " .. CONF_PATH .. ": " .. tostring(err)) + end + + for line in f:lines() do + parse_line(line) + end + + f:close() +end + +return { + nameservers = nameservers, + search = search, + ndots = ndots, +} diff --git a/rootfs/etc/nginx/template/nginx.tmpl b/rootfs/etc/nginx/template/nginx.tmpl index 252db4cd6..d378a036d 100755 --- a/rootfs/etc/nginx/template/nginx.tmpl +++ b/rootfs/etc/nginx/template/nginx.tmpl @@ -77,7 +77,6 @@ http { error("require failed: " .. tostring(res)) else configuration = res - configuration.nameservers = { {{ buildResolversForLua $cfg.Resolver $cfg.DisableIpv6DNS }} } end ok, res = pcall(require, "balancer") @@ -623,7 +622,6 @@ stream { error("require failed: " .. tostring(res)) else configuration = res - configuration.nameservers = { {{ buildResolversForLua $cfg.Resolver $cfg.DisableIpv6DNS }} } end ok, res = pcall(require, "tcp_udp_configuration") diff --git a/test/e2e/lua/dynamic_configuration.go b/test/e2e/lua/dynamic_configuration.go index c687f0ae4..f3903fe8a 100644 --- a/test/e2e/lua/dynamic_configuration.go +++ b/test/e2e/lua/dynamic_configuration.go @@ -20,7 +20,6 @@ import ( "crypto/tls" "fmt" "net/http" - "regexp" "strings" "time" @@ -63,13 +62,6 @@ var _ = framework.IngressNginxDescribe("Dynamic Configuration", func() { }) }) - It("sets nameservers for Lua", func() { - f.WaitForNginxConfiguration(func(cfg string) bool { - r := regexp.MustCompile(`configuration.nameservers = { [".,0-9a-zA-Z]+ }`) - return r.MatchString(cfg) - }) - }) - Context("Lua shared dict", func() { It("update config", func() {