Rate Limit policy: allow templating using liquid

[y-tabata]

Closes #713.

[davidor]

I think this should be more descriptive.

[davidor]

@y-tabata Thanks for your contribution! 👍

Can you add a changelog entry, please? Something like Liquid templating support in the rate-limit policy under the Added section would be enough.

Also, could you add some unit tests for this? I see that you added some integration tests which is good 👍 , but we also need the unit ones. You can use the ones we added for the headers policy as an example: #716

[davidor]

Why is this needed?

[y-tabata]

In order to resolve 'fixtures.rsa'.
I referenced apicast-oidc.t.

[davidor]

I see. I think we can find a better solution. I think these tests should make use of the fixtures directory in t instead of the one in spec. However, as you said, we're doing the same in apicast-oidc.t. We should fix this, but I think we can do it after this PR.

[davidor]

Your solution works, but I think there's a simpler way to solve the same problem.

What we want to do is to make ngx.var.uri and ngx.var.host available in the liquid templates.

The regex matching is not necessary. We can achieve our goal by including new keys in the context we pass to template_string:render(context). If that context includes a table with these keys uri = ngx.var.uri and host = ngx.var.host we'll be able to use {{ uri }} and {{ host }} in the config instead of {{ ngx.var.uri }} and {{ ngx.var.host }}.

Another option is to include ngx.var.host in the context that we send to template_string:render(). However, I think that when configuring this from the 3scale UI, users will find it more natural to write {{host}} than to write {{ngx.var.host}}. In theory, they don't even need to know that apicast is based on nginx. I had a discussion with @mikz about this in another PR: #716 (comment)

[y-tabata]

If we don't use the regex, we have to have the list like uri = ngx.var.uri or host = ngx.var.host and so on.
The best place is liquid_filters of template_string.lua, isn't it?
Do you have any idea to avoid the "API disabled in the current context" error?

[davidor]

I don't think liquid_filters in template_string.lua is the best place for this. Those are filters, functions like md5 that allow us to have things like {{ 'a_value' | md5 }} in the policy config.

But uri and host are values, not filters. I think uri and host need to be included in the context passed to render. For that, I think a possible solution is to have a table somewhere with the values we want to have available in the context. As a starting point, we could have something like:

local function context_values()
  return {
    uri = ngx.var.uri,
    host = ngx.var.host,
    -- possibly many others
  }
end

_M.available_context(policies_context)
  -- merge policies_context with context_values and return the result
end

Maybe we could include this in the Policy module or even in a new separate module.

If we evaluate the ngx.var* vars in access(), we won't see the API disabled in the current context error.

[y-tabata]

Changed.

[mikz]

pairs method can't be JITed, it is always better to use ipairs and change the data structure.

In this case it is not even needed. What are you doing is mutating the passed policies_context. That is really not expected. It is way better to return a new object.

You don't need to even copy the context table, just use metatable __index property to delegate unknown fields:

return setmetatable(context_values(), { __index = policies_context })

[mikz]

This mutates the limiter object which is shared across all requests.

You can just pass context to build_limiters_and_keys and use it there without storing it in the shared limiter object.

[y-tabata]

Do you have any idea to avoid prove errors?
These tests succeeded in my local environment, and I remember 3 days ago the tests succeeded here, too.

[mikz]

This still mutates the context. It should be the other way around. Setting the metatable of context_values.

[y-tabata]

Fixed.

[mikz]

@y-tabata the tests fail because of #719 (comment)

[y-tabata]

@mikz thanks!

[mikz]

👍 good job!

[davidor]

Looks good 👍
Thanks for your contribution @y-tabata ! 👍