diff --git a/CHANGELOG.md b/CHANGELOG.md index 7c068ca4c..f3bdc1cdf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,9 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ## [Unreleased] +### Performance +- Camel Policy: avoid unnecessary closure allocation per request [PR #1584](https://github.com/3scale/APIcast/pull/1584) + ### Fixed - Correct FAPI header to `x-fapi-interaction-id` [PR #1557](https://github.com/3scale/APIcast/pull/1557) [THREESCALE-11957](https://issues.redhat.com/browse/THREESCALE-11957) - Only validate oidc setting if authentication method is set to oidc [PR #1568](https://github.com/3scale/APIcast/pull/1568) [THREESCALE-11441](https://issues.redhat.com/browse/THREESCALE-11441) diff --git a/gateway/src/apicast/policy/camel/camel.lua b/gateway/src/apicast/policy/camel/camel.lua index 8e1a10434..65a19be60 100644 --- a/gateway/src/apicast/policy/camel/camel.lua +++ b/gateway/src/apicast/policy/camel/camel.lua @@ -1,5 +1,5 @@ local policy = require('apicast.policy') -local _M = policy.new('http_proxy', 'builtin') +local _M = policy.new('camel_proxy', 'builtin') local resty_url = require 'resty.url' local ipairs = ipairs @@ -21,19 +21,26 @@ function _M.new(config) end for _, proto in ipairs(proxies) do - local val, err = resty_url.parse(config[string.format("%s_proxy", proto)]) - if err then - ngx.log(ngx.WARN, proto, " proxy is not correctly defined, err: ", err) + local proxy_url = config[string.format("%s_proxy", proto)] + if proxy_url then + local val, err = resty_url.parse(proxy_url) + if err then + ngx.log(ngx.WARN, proto, " proxy is not correctly defined, err: ", err) + end + self.proxies[proto] = val + else + self.proxies[proto] = self.all_proxy end - self.proxies[proto] = val or self.all_proxy + end + -- This get_http_proxy function will be called in upstream just in case if a + -- proxy is defined. + self.get_http_proxy = function(uri) + if not uri.scheme then return nil end + return self.proxies[uri.scheme] end return self end -local function find_proxy(self, scheme) - return self.proxies[scheme] -end - function _M:access(context) context.skip_https_connect_on_proxy = true @@ -48,12 +55,7 @@ end function _M:rewrite(context) -- This get_http_proxy function will be called in upstream just in case if a -- proxy is defined. - context.get_http_proxy = function(uri) - if not uri.scheme then - return nil - end - return find_proxy(self, uri.scheme) - end + context.get_http_proxy = self.get_http_proxy end return _M diff --git a/spec/policy/camel/camel_spec.lua b/spec/policy/camel/camel_spec.lua index 3464616fe..2b468f473 100644 --- a/spec/policy/camel/camel_spec.lua +++ b/spec/policy/camel/camel_spec.lua @@ -58,10 +58,48 @@ describe('Camel policy', function() it("invalid protocol", function() proxy:rewrite(context) - local result = context:get_http_proxy( - {}, {scheme="invalid"}) + local result = context.get_http_proxy({scheme="invalid"}) assert.is_nil(result) end) + it("nil scheme", function() + proxy:rewrite(context) + local result = context.get_http_proxy({}) + assert.is_nil(result) + end) + + end) + + describe(".access", function() + it("sets skip_https_connect_on_proxy on context", function() + local proxy = camel_policy.new({ all_proxy = all_proxy_val }) + + local mock_upstream = { set_skip_https_connect_on_proxy = stub.new() } + context.get_upstream = function() return mock_upstream end + + proxy:access(context) + + assert.is_true(context.skip_https_connect_on_proxy) + end) + + it("calls set_skip_https_connect_on_proxy on upstream", function() + local proxy = camel_policy.new({ all_proxy = all_proxy_val }) + + local mock_upstream = { set_skip_https_connect_on_proxy = stub.new() } + context.get_upstream = function() return mock_upstream end + + proxy:access(context) + + assert.stub(mock_upstream.set_skip_https_connect_on_proxy).was_called() + end) + + it("does not error when get_upstream returns nil", function() + local proxy = camel_policy.new({ all_proxy = all_proxy_val }) + + context.get_upstream = function() return nil end + + assert.has_no.errors(function() proxy:access(context) end) + assert.is_true(context.skip_https_connect_on_proxy) + end) end) end)