From c80c6d6dfb077999e7a629a52458d05719a8c70a Mon Sep 17 00:00:00 2001 From: An Tran Date: Tue, 28 May 2024 14:05:11 +1000 Subject: [PATCH 1/3] [refactor] Small refactor to use read_file from util --- .../src/apicast/configuration_loader/file.lua | 3 +- gateway/src/apicast/management.lua | 3 +- gateway/src/apicast/policy/tls/tls.lua | 34 ++----------------- .../policy/upstream_mtls/upstream_mtls.lua | 22 +++--------- gateway/src/apicast/util.lua | 21 +++++------- 5 files changed, 19 insertions(+), 64 deletions(-) diff --git a/gateway/src/apicast/configuration_loader/file.lua b/gateway/src/apicast/configuration_loader/file.lua index 85b23f05b..9858e7a63 100644 --- a/gateway/src/apicast/configuration_loader/file.lua +++ b/gateway/src/apicast/configuration_loader/file.lua @@ -1,7 +1,6 @@ local len = string.len local format = string.format local tostring = tostring -local open = io.open local assert = assert local sub = string.sub local util = require 'apicast.util' @@ -39,7 +38,7 @@ local function is_path(path) end local function read_path(path) - return assert(open(path)):read('*a') + return assert(util.read_file(path)) end local function read(path) diff --git a/gateway/src/apicast/management.lua b/gateway/src/apicast/management.lua index ea7bd1be2..b0eb93da0 100644 --- a/gateway/src/apicast/management.lua +++ b/gateway/src/apicast/management.lua @@ -9,6 +9,7 @@ local inspect = require('inspect') local resolver_cache = require('resty.resolver.cache') local env = require('resty.env') local policy_manifests_loader = require('apicast.policy_manifests_loader') +local util = require('apicast.util') local policy_loader = require('apicast.policy_loader') @@ -66,7 +67,7 @@ function _M.update_config() local file = ngx.req.get_body_file() if not data then - data = assert(io.open(file)):read('*a') + data = assert(util.read_file(file)) end local config, err = configuration_parser.decode(data) diff --git a/gateway/src/apicast/policy/tls/tls.lua b/gateway/src/apicast/policy/tls/tls.lua index 8c0d14c03..78733f87b 100644 --- a/gateway/src/apicast/policy/tls/tls.lua +++ b/gateway/src/apicast/policy/tls/tls.lua @@ -6,10 +6,9 @@ local tab_new = require('table.new') local ssl = require('ngx.ssl') local cjson = require('cjson') local data_url = require('resty.data_url') +local util = require('apicast.util') local insert = table.insert -local io_open = io.open -local io_type = io.type local pack = table.pack local ipairs = ipairs local setmetatable = setmetatable @@ -48,33 +47,6 @@ end local EmbeddedCertificates = Config('certificate', 'certificate_key') local LocalFilesystemCertificates = Config('certificate_path', 'certificate_key_path') -local function open_file(path) - local handle, err - - if io_type(path) == 'handle' then - handle = path - else - handle, err = io_open(path) - end - - return handle, err -end - -local function read_file(path) - local handle, err = open_file(path) - - if err or not handle then - return nil, err - end - - handle:seek("set") - local output = handle:read("*a") - handle:close() - - return output -end - - local function parse_certificates(self, certificate, private_key) local err self.certificate, err = ssl.parse_pem_cert(certificate) @@ -110,10 +82,10 @@ end function LocalFilesystemCertificates:parse() local certificate, certificate_key, err - certificate, err = read_file(self.certificate_path) + certificate, err = util.read_file(self.certificate_path) if err then return nil, err end - certificate_key, err = read_file(self.certificate_key_path) + certificate_key, err = util.read_file(self.certificate_key_path) if err then return nil, err end return parse_certificates(self, certificate, certificate_key) diff --git a/gateway/src/apicast/policy/upstream_mtls/upstream_mtls.lua b/gateway/src/apicast/policy/upstream_mtls/upstream_mtls.lua index c376d2a1e..ade9393bc 100644 --- a/gateway/src/apicast/policy/upstream_mtls/upstream_mtls.lua +++ b/gateway/src/apicast/policy/upstream_mtls/upstream_mtls.lua @@ -4,10 +4,10 @@ local ssl = require('ngx.ssl') local ffi = require "ffi" local base = require "resty.core.base" local data_url = require('resty.data_url') +local util = require 'apicast.util' local C = ffi.C local get_request = base.get_request -local open = io.open local pairs = pairs local X509_STORE = require('resty.openssl.x509.store') @@ -31,25 +31,11 @@ local embedded_type = "embedded" local new = _M.new - -local function read_file(path) - ngx.log(ngx.DEBUG, "reading path:", path) - - local file = open(path, "rb") - if not file then - ngx.log(ngx.ERR, "Cannot read path: ", path) - return nil - end - - local content = file:read("*a") - file:close() - return content -end - - local function get_cert(value, value_type) + if value_type == path_type then - return read_file(value) + ngx.log(ngx.DEBUG, "reading path:", value) + return util.read_file(value) end if value_type == embedded_type then diff --git a/gateway/src/apicast/util.lua b/gateway/src/apicast/util.lua index 091d4abbf..e435f00f5 100644 --- a/gateway/src/apicast/util.lua +++ b/gateway/src/apicast/util.lua @@ -22,17 +22,14 @@ function _M.timer(name, fun, ...) return unpack(ret) end -local function read(file) - local handle, err = open(file) - local output - - if handle then - output = handle:read("*a") - handle:close() - else - return nil, err +function _M.read_file(file) + local handle, err = open(file, 'r') + if not handle then return nil, err end + local output, read_err = handle:read("*a") + handle:close() + if not output then + return nil, read_err end - return output end @@ -46,13 +43,13 @@ function _M.system(command) local success, exit, code = execute('(' .. command .. ')' .. ' > ' .. tmpout .. ' 2> ' .. tmperr) local err - tmpout, err = read(tmpout) + tmpout, err = _M.read_file(tmpout) if err then return nil, err end - tmperr, err = read(tmperr) + tmperr, err = _M.read_file(tmperr) if err then return nil, err From f4c638dd1a14da2a42f468ce04d14f87aa533b2c Mon Sep 17 00:00:00 2001 From: An Tran Date: Tue, 28 May 2024 14:20:12 +1000 Subject: [PATCH 2/3] [refactor] Replace os.execute with penlight executeex function --- gateway/src/apicast/util.lua | 30 +++++++----------------------- 1 file changed, 7 insertions(+), 23 deletions(-) diff --git a/gateway/src/apicast/util.lua b/gateway/src/apicast/util.lua index e435f00f5..d05e34f89 100644 --- a/gateway/src/apicast/util.lua +++ b/gateway/src/apicast/util.lua @@ -9,9 +9,8 @@ local sub = string.sub local errlog = require('ngx.errlog') local open = io.open -local execute = os.execute -local tmpname = os.tmpname local unpack = unpack +local pl_utils = require "pl.utils" function _M.timer(name, fun, ...) local start = ngx_now() @@ -36,40 +35,25 @@ end local max_log_line_len = 4096-96 -- 96 chars for our error message function _M.system(command) - local tmpout = tmpname() - local tmperr = tmpname() + command = '(' .. command ..')' ngx.log(ngx.DEBUG, 'os execute ', command) - - local success, exit, code = execute('(' .. command .. ')' .. ' > ' .. tmpout .. ' 2> ' .. tmperr) - local err - - tmpout, err = _M.read_file(tmpout) - - if err then - return nil, err - end - - tmperr, err = _M.read_file(tmperr) - - if err then - return nil, err - end + local success, retcode, stdout, stderr = pl_utils.executeex(command) -- os.execute returns exit code as first return value on OSX -- even though the documentation says otherwise (true/false) if success == 0 or success == true then - local max = len(tmperr) + local max = len(stderr) if max > 0 then errlog.raw_log(ngx.WARN, 'os execute stderr:') for start=0, max , max_log_line_len do - errlog.raw_log(ngx.WARN, sub(tmperr, start, start + max_log_line_len - 1)) + errlog.raw_log(ngx.WARN, sub(stderr, start, start + max_log_line_len - 1)) end end - return tmpout + return stdout else - return tmpout, tmperr, code or exit or success + return stdout, stderr, retcode or success end end From 2e6e6dddf065fcc95d22f16785597e910a5f0a13 Mon Sep 17 00:00:00 2001 From: An Tran Date: Tue, 28 May 2024 15:34:10 +1000 Subject: [PATCH 3/3] [refactor] Small refactor and remove unsed code --- .../policy/upstream_mtls/upstream_mtls.lua | 29 +++++-------------- 1 file changed, 8 insertions(+), 21 deletions(-) diff --git a/gateway/src/apicast/policy/upstream_mtls/upstream_mtls.lua b/gateway/src/apicast/policy/upstream_mtls/upstream_mtls.lua index ade9393bc..cbc714457 100644 --- a/gateway/src/apicast/policy/upstream_mtls/upstream_mtls.lua +++ b/gateway/src/apicast/policy/upstream_mtls/upstream_mtls.lua @@ -58,21 +58,14 @@ local function read_certificate(value, value_type) end local function read_certificate_key(value, value_type) - local data = get_cert(value, value_type) - if data == nil then - ngx.log(ngx.ERR, "Certificate value is invalid") - return - end - if data == nil then ngx.log(ngx.ERR, "Certificate key value is invalid") return end return ssl.parse_pem_priv_key(data) - end local function read_ca_certificates(ca_certificates) @@ -115,13 +108,7 @@ end -- Set the certs for the upstream connection. Need to receive the pointers from -- parse_* functions. --- Public function to be able to unittest this. -function _M.set_certs(cert, key) - local r = get_request() - if not r then - ngx.log(ngx.ERR, "Invalid request") - return - end - +function _M.set_certs(r, cert, key) local val = C.ngx_http_apicast_ffi_set_proxy_cert_key(r, cert, key) if val ~= ngx.OK then ngx.log(ngx.ERR, "Certificate cannot be set correctly") @@ -140,17 +127,17 @@ end --to @upstream, so the request need to be the one that connects to the --upstream0 function _M:balancer(context) - if self.cert and self.cert_key then - self.set_certs(self.cert, self.cert_key) + local r = get_request() + if not r then + ngx.log(ngx.WARN, "Invalid request") + return end - if not self.verify then - return + if self.cert and self.cert_key then + self.set_certs(r, self.cert, self.cert_key) end - local r = get_request() - if not r then - ngx.log(ngx.WARN, "Invalid request") + if not self.verify then return end