Fix bug when downloading configs that contain Liquid #845
Conversation
| @@ -317,8 +317,10 @@ function _M:config(service, environment, version) | |||
| if res.status == 200 then | |||
| local proxy_config = cjson.decode(res.body).proxy_config | |||
|
|
|||
| local config_service = configuration.parse_service(proxy_config.content) | |||
| local issuer, oidc_config = self:oidc_issuer_configuration(config_service) | |||
| local oidc_issuer_endpoint = proxy_config.content.proxy and | |||
There was a problem hiding this comment.
Note: when calling parse_service, the configuration module builds the policy chain, and when doing so, in the case of the headers policy, the configuration received in new() is modified because it adds a template_string field with an instance of TemplateString.
That's why this failed only when the service was parsed. Parsing the service is not a problem by itself, but later, when the proxy configs are encoded, it fails because of the template string.
An unexpected side-effect.
We should probably review the policy initializers and avoid this kind of side-effects.
There was a problem hiding this comment.
We will need an integration test to reproduce this failure.
And I wonder why it crashes when we add an instance of TemplateString.
There was a problem hiding this comment.
I'll add an integration test.
Regarding the TemplateString exception, here's a minimal case to reproduce it:
local cjson = require 'cjson'
local Liquid = require 'liquid'
local LiquidTemplate = Liquid.Template
local template = LiquidTemplate:parse('{{ service.id }}')
cjson.encode(template) -- Error: Cannot serialise table: table key must be a number or stringThere was a problem hiding this comment.
I see, the configuration loader tries to encode back the table into string:
Then we could do the deepclone just before parsing the service:
https://github.com/3scale/apicast/blob/635590f8dedd895cbba4288faaecc13854967781/gateway/src/apicast/configuration_loader/remote_v2.lua#L119
There was a problem hiding this comment.
That would work too 👍
There was a problem hiding this comment.
At first I thought that there was no point in parsing the service, because the information can be easily get from the JSON. But maybe parsing the service is better in case we're performing some validations there.
davidor
force-pushed
the
fix-bug-remote-config-with-liquid
branch
from
fa47fd0
to
3eb5515
Compare
|
@davidor is there any estimate when this will be merged? (I suppose next week?) |
| @@ -5,6 +5,8 @@ | |||
| -- Similarly, this policy also allows to add, modify, and delete the headers | |||
| -- included in the response. | |||
|
|
|||
| local tablex = require('pl.tablex') | |||
There was a problem hiding this comment.
I really don't want to depend on penlight in policies. OpenResty provides table.clone that does shallow clone.
Also, if we are doing this, then it should be pushed from the top, not done in each policy.
There was a problem hiding this comment.
OK. I suggest we solve the problem in the loader first and leave the immutable config for a future PR.
davidor
force-pushed
the
fix-bug-remote-config-with-liquid
branch
from
635590f
to
8077a7e
Compare
We were parsing the service just to get the OIDC issuer endpoint. That's unnecessary because we can get easily get it from the decoded JSON.
Also, parsing the service was causing a bug. An error was raised with services containing a policy with fields to be interpreted by Liquid (have '{{' and '}}'). (Closes #828 )