-
Notifications
You must be signed in to change notification settings - Fork 171
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 string
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would work too 👍
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.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. I suggest we solve the problem in the loader first and leave the immutable config for a future PR.
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.
Agreed 👍
635590f
to
8077a7e
Compare
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.
👍
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 )