Headers policy: allow templating using liquid #716
Conversation
davidor
force-pushed
the
liquid-templating-headers-policy
branch
from
59410f5
to
faa2735
Compare
| -- Expose only ngx.* functions that we think could be useful and that do not | ||
| -- have any side-effects. | ||
| local liquid_filters = { | ||
| ngx_excape_uri = ngx.escape_uri, |
There was a problem hiding this comment.
I'm torn on this.
On one hand customer should not care if it is ngx or what. All functions to escape_uri should behave the same.
On the other hand it is the nginx function and in theory we might provide some other in the future.
Also, would be good to add ldoc with links to those functions so we can see it in the documentation.
There was a problem hiding this comment.
@mikz I had the same doubt while writing this code.
I'm thinking about users configuring this from 3scale's UI. I think that for them it'd be more natural to write something like 'md5' rather than 'ngx_md5'. In theory, they don't even need to know that apicast is based on nginx.
| -- Can be 'liquid' or 'plain' | ||
| -- @treturn a template string | ||
| function _M.new(value, type) | ||
| return template[type].new(value) |
There was a problem hiding this comment.
This will crash when it gets unknown type. It should handle missing types and return nil, err IMO.
|
@davidor might be good as a verification to try to replicate https://github.com/nmasse-itix/3scale-apicast/blob/34a01da49ddb90dcbb85027b0d54be503d8ed236/gateway/src/custom/policy/explode_oidc_token_as_http_headers.lua with this. |
davidor
force-pushed
the
liquid-templating-headers-policy
branch
from
faa2735
to
33e891e
Compare
davidor
force-pushed
the
liquid-templating-headers-policy
branch
2 times, most recently
from
5c68091
to
be687f9
Compare
|
@mikz I added an integration test with jwt tokens as you suggested. Check the last 2 commits. |
| @@ -0,0 +1,23 @@ | |||
| local http_authorization = require 'resty.http_authorization' | |||
There was a problem hiding this comment.
@mikz after doing this, I wondered whether it would be better to just expose the jwt token in the policies context instead of creating this fixture policy specifically for this. So I went ahead and did that in a separate branch based on this one: https://github.com/3scale/apicast/tree/expose-jwt-tokens-context (4 last commits)
Let's discuss and decide what we want to include in this PR.
There was a problem hiding this comment.
@mikz I'll try to explain this better.
In order to test that the templating introduced in this PR allows setting headers with some jwt information, I needed to create an example policy specifically for testing that. The reason is that none of the current policies expose a decoded jwt in the policies context.
After adding that example policy, I realized that we should probably expose the decoded jwt in the policies context so other policies can use it, for example, the rate-limit one. That's why I created a new branch that does that.
My suggestion is to delete the commits of this branch that implement the example policy and the test (last 2 commits), merge, and then, create a new PR from the new branch I created.
What do you think?
There was a problem hiding this comment.
Makes total sense 👍
I thought jwt is already exposed. If it is not then we definitely should do it
There was a problem hiding this comment.
Done 👍
I'll continue working on the other PR tomorrow.
davidor
force-pushed
the
liquid-templating-headers-policy
branch
from
be687f9
to
6ec4307
Compare
Closes #708