Add 3scale referrer policy #728
Conversation
davidor
force-pushed
the
3scale-referrer-policy
branch
from
d1a76dc to
3d97208
Compare
| return self | ||
| end | ||
|
|
||
| function _M.rewrite(_, context) |
There was a problem hiding this comment.
It'd be nice to be able to use export instead of rewrite() but can't because we need to add to the existing table, not overwrite it.
| @@ -0,0 +1,20 @@ | |||
| local policy = require('apicast.policy') | |||
| local _M = policy.new('3scale Referrer policy') | |||
There was a problem hiding this comment.
I added the 3scale prefix here because this is 3scale specific. Like the 3scale_batcher policy.
| -- Params to send in 3scale backend calls that are not the typical ones | ||
| -- (credentials, usage, etc.). | ||
| -- This allows us, for example, to send a referrer. | ||
| extra_params_backend_authrep = {} |
There was a problem hiding this comment.
I opted for this because it's way easier than propagating these extra params to all the methods that need them.
I think this makes the code more readable.
There was a problem hiding this comment.
I guess it is fine. Proxy object mutates internal state anyway, so it is not really ready to be used by multiple clients.
| local referrer = ngx.var.http_referer | ||
|
|
||
| if referrer then | ||
| context.extra_params_backend_authrep = context.extra_params_backend_authrep or {} |
There was a problem hiding this comment.
We need to find a better way to handle those. They are basically global variables and it starts becoming really ugly.
There was a problem hiding this comment.
Can't this policy manipulate the proxy object directly?
There was a problem hiding this comment.
Yes, but the proxy object should also be in context (stored by local_chain.lua). Do you think that's a better solution?
There was a problem hiding this comment.
I think it already is there.
There was a problem hiding this comment.
Yes. The proxy is stored in context in local_chain.lua (edited my comment at the same time you were writing).
Now that I think about it, I feel like manipulating here the proxy object is a slightly better solution because it avoids the extra step of doing it in the APIcast policy. Instead of doing that, every policy would be responsible for adding the extra backend params it wants. Is that the benefit you were thinking about?
There was a problem hiding this comment.
Yes, that would reduce the number of objects we store in the context root. And then changing the params basically becomes proxy public API.
|
|
||
| local new = _M.new | ||
|
|
||
| function _M.new(config) |
There was a problem hiding this comment.
We don't even need to override new, right?
There was a problem hiding this comment.
You're right. This is the first policy we have with an empty configuration, so we don't need to override new. Didn't realize that 👍
davidor
force-pushed
the
3scale-referrer-policy
branch
from
04bd58b to
b60284f
Compare
davidor
force-pushed
the
3scale-referrer-policy
branch
from
b60284f to
35adacd
Compare
|
@mikz I addressed your comments. Ready for review. |
Closes #414