Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
41 changes: 21 additions & 20 deletions gateway/src/apicast/configuration_loader/remote_v2.lua
Original file line number Diff line number Diff line change
Expand Up @@ -102,33 +102,36 @@ local function service_config_endpoint(portal_endpoint, service_id, env, version
)
end

local function get_oidc_issuer_endpoint(proxy_content)
return proxy_content.proxy and proxy_content.proxy.oidc_issuer_endpoint
end

local function parse_proxy_configs(self, proxy_configs)
local config = { services = array(), oidc = array() }

for i, proxy_conf in ipairs(proxy_configs) do
local proxy_config = proxy_conf.proxy_config
local content = proxy_config.content

-- Copy the config because parse_service have side-effects. It adds
-- liquid templates in some policies and those cannot be encoded into a
-- JSON. We should get rid of these side effects.
local original_proxy_config = deepcopy(proxy_config)

local service = configuration.parse_service(proxy_config.content)
config.services[i] = content

local issuer_endpoint = get_oidc_issuer_endpoint(content)
local oidc
if issuer_endpoint then
oidc = self.oidc:call(issuer_endpoint, self.ttl)
end
-- We always assign a oidc to the service, even an empty one with the
-- service_id, if not on APICAST_SERVICES_LIST will fail on filtering
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we need to leave this comment to explain the reason?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

local oidc = self:oidc_issuer_configuration(service)
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like the oidc_issuer_configuration function is not called from anywhere anymore, so probably can be deleted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

if not oidc then
oidc = {}
end

-- deepcopy because this can be cached, and we want to have a deepcopy to
-- avoid issues with service_id
local oidc_copy = deepcopy(oidc)
oidc_copy.service_id = service.id
oidc_copy.service_id = tostring(content.id)

config.oidc[i] = oidc_copy
config.services[i] = original_proxy_config.content
end
return cjson.encode(config)
end
Expand Down Expand Up @@ -451,10 +454,6 @@ function _M:services()
return services
end

function _M:oidc_issuer_configuration(service)
return self.oidc:call(service.oidc.issuer_endpoint, self.ttl)
end

function _M:config(service, environment, version, service_regexp_filter)
local http_client = self.http_client

Expand Down Expand Up @@ -482,20 +481,22 @@ function _M:config(service, environment, version, service_regexp_filter)

if res.status == 200 then
local proxy_config = cjson.decode(res.body).proxy_config

-- Copy the config because parse_service have side-effects. It adds
-- liquid templates in some policies and those cannot be encoded into a
-- JSON. We should get rid of these side effects.
local original_proxy_config = deepcopy(proxy_config)
local content = proxy_config.content

local config_service = configuration.parse_service(proxy_config.content)
if service_regexp_filter and not config_service:match_host(service_regexp_filter) then
return nil, "Service filtered out because APICAST_SERVICES_FILTER_BY_URL"
end

original_proxy_config.oidc = self:oidc_issuer_configuration(config_service)
local issuer_endpoint = get_oidc_issuer_endpoint(content)
local oidc

if issuer_endpoint then
oidc = self.oidc:call(issuer_endpoint, self.ttl)
end

return original_proxy_config
proxy_config.oidc = oidc
return proxy_config
else
return nil, status_code_error(res)
end
Expand Down
9 changes: 6 additions & 3 deletions gateway/src/apicast/policy_chain.lua
Original file line number Diff line number Diff line change
Expand Up @@ -193,12 +193,15 @@ function _M:add_policy(name, version, ...)
end
end

local default_policy_order_check = PolicyOrderChecker.new(policy_manifests_loader.get_all())

-- Checks if there are any policies placed in the wrong place in the chain.
-- It doesn't return anything, it prints error messages when there's a problem.
function _M:check_order(manifests)
PolicyOrderChecker.new(
manifests or policy_manifests_loader.get_all()
):check(self)
if manifests then
return PolicyOrderChecker.new(manifests):check(self)
end

Choose a reason for hiding this comment

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

A nit, it looks like the method is only called in gateway/src/apicast/configuration.lua without parameters. So perhaps makes sense to simplify by removing the argument from it as well the if clause.

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably the idea was to use the manifest cache somewhere?

In any case - why is this change implemented? 🤔

It seems to me that:

  1. It always loads all policies (on line :196) regardless of whether manifests is passed or not.
  2. If manifests is passed, and is not null (as @akostadinov pointed out - this does not happen at this point), then the policy order check will be performed 2 times. Is that intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch.

Choose a reason for hiding this comment

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

Still why do we need this condition if we never call the method with parameters? Anyway, not super important as I said.

default_policy_order_check:check(self)
end

local function call_chain(phase_name)
Expand Down
34 changes: 32 additions & 2 deletions gateway/src/apicast/policy_config_validator.lua
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,35 @@
-- Validates a policy configuration against a policy config JSON schema.

local jsonschema = require('jsonschema')
local lrucache = require("resty.lrucache")

local _M = { }
local cached_validator = lrucache.new(100)

local _M = {
_VERSION=0.1
}

local function create_validator(schema)
local ok, res = pcall(jsonschema.generate_validator, schema)
if ok then
return res
end

return nil, res
end

local function get_validator(schema)
local validator, err = cached_validator:get(schema)
if not validator then
validator, err = create_validator(schema)
if not validator then
return nil, err
end
cached_validator:set(schema, validator)
end

return validator, nil
end

--- Validate a policy configuration
-- Checks if a policy configuration is valid according to the given schema.
Expand All @@ -13,7 +40,10 @@ local _M = { }
-- @treturn boolean True if the policy configuration is valid. False otherwise.
-- @treturn string Error message only when the policy config is invalid.
function _M.validate_config(config, config_schema)
local validator = jsonschema.generate_validator(config_schema or {})
local validator, err = get_validator(config_schema or {})
if not validator then
return false, err
end
return validator(config or {})
end

Expand Down
44 changes: 37 additions & 7 deletions gateway/src/apicast/policy_loader.lua
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,11 @@ local concat = table.concat
local setmetatable = setmetatable
local pcall = pcall

local isempty = require('table.isempty')

-- Module-level cache storage (one per worker process)
local manifests_cache = {}

local _M = {}

local resty_env = require('resty.env')
Expand Down Expand Up @@ -70,8 +75,26 @@ local function lua_load_path(load_path)
return format('%s/?.lua', load_path)
end

-- Get a cached manifest by policy name and version
-- @tparam string name The policy name
-- @tparam string version The policy version
-- @treturn table|nil The cached manifest table, or nil if not cached
local function get_cached_manifest(name, version)
local manifests = manifests_cache[name]
if manifests then
for _, manifest in ipairs(manifests) do
if version == manifest.version then
return manifest
end
end
end
end

local function load_manifest(name, version, path)
local manifest = read_manifest(path)
local manifest = get_cached_manifest(name, version)
if not manifest then
manifest = read_manifest(path)
end

if manifest then
if manifest.version ~= version then
Expand Down Expand Up @@ -110,8 +133,8 @@ end
function _M:load_path(name, version, paths)
local failures = {}

for _, path in ipairs(paths or self.policy_load_paths()) do
local manifest, load_path = load_manifest(name, version, format('%s/%s/%s', path, name, version) )
if version == 'builtin' then
local manifest, load_path = load_manifest(name, version, format('%s/%s', self.builtin_policy_load_path(), name) )

if manifest then
return load_path, manifest.configuration
Expand All @@ -120,8 +143,8 @@ function _M:load_path(name, version, paths)
end
end

if version == 'builtin' then
local manifest, load_path = load_manifest(name, version, format('%s/%s', self.builtin_policy_load_path(), name) )
for _, path in ipairs(paths or self.policy_load_paths()) do
local manifest, load_path = load_manifest(name, version, format('%s/%s/%s', path, name, version) )

if manifest then
return load_path, manifest.configuration
Expand All @@ -130,6 +153,7 @@ function _M:load_path(name, version, paths)
end
end


return nil, nil, failures
end

Expand Down Expand Up @@ -173,9 +197,15 @@ end
-- Returns all the policy modules
function _M:get_all()
local policy_modules = {}
local manifests

local policy_manifests_loader = require('apicast.policy_manifests_loader')
local manifests = policy_manifests_loader.get_all()
if isempty(manifests_cache) then
local policy_manifests_loader = require('apicast.policy_manifests_loader')
manifests = policy_manifests_loader.get_all()
manifests_cache = manifests
else
manifests = manifests_cache
end

for policy_name, policy_manifests in pairs(manifests) do
for _, manifest in ipairs(policy_manifests) do
Expand Down
2 changes: 1 addition & 1 deletion spec/configuration_loader/remote_v2_spec.lua
Original file line number Diff line number Diff line change
Expand Up @@ -651,7 +651,7 @@ UwIDAQAB
it('does not crash on empty issuer', function()
local service = { oidc = { issuer_endpoint = '' }}

assert.falsy(loader:oidc_issuer_configuration(service))
assert.falsy(loader.oidc:call(service.oidc.issuer_endpoint, 0))
end)
end)

Expand Down
Loading