-
Notifications
You must be signed in to change notification settings - Fork 172
THREESCALE-12133 optimize configuration reload #1564
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.
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
base: master
Are you sure you want to change the base?
Changes from all commits
aa7346b
ad0342d
771ea26
2a13c8e
e5d8629
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
| local oidc = self:oidc_issuer_configuration(service) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
|
@@ -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 | ||
|
|
||
|
|
@@ -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 | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A nit, it looks like the method is only called in
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
|
|
||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done